Define CMake args and environment variables simultaneously in install script

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?

[jathu]

Summary

This script currently configures CMAKE_ARGS, that is eventually propagated to the underlying cmake build (via pip install). However, notice how for some args, we also define an equivalent envvar. This is primarily for the pip setup script:

executorch/setup.py

Lines 72 to 98 in 78ee0e6

	classShouldBuild:
	"""Indicates whether to build various components."""
	@staticmethod
	def_is_env_enabled(env_var: str, default: bool=False) ->bool:
	val=os.environ.get(env_var, None)
	ifvalisNone:
	returndefault
	ifvalin ("OFF", "0", ""):
	returnFalse
	returnTrue
	@classmethod
	defpybindings(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_PYBIND", default=False)
	@classmethod
	deftraining(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_TRAINING", default=False)
	@classmethod
	defllama_custom_ops(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT", default=True)
	@classmethod
	defflatc(cls) ->bool:
	returncls._is_env_enabled("EXECUTORCH_BUILD_FLATC", default=True)

So there is an implicit expectation that if you want to access these definitions in the setup script, you need to make sure to also define the envvar along with the cmake args. Well, it took me more than an hour to figure out why my cmake args wasn't being recognized in the setup script 🤦 . So, to make sure consistency — let's force setting args and envvars simultaneously.

Test plan

build + check CMakeCache.txt to ensure flags are set

# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=OFF
$ rm -rf pip-out dist && ./install_executorch.sh
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=OFF EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind coreml
# Expected: EXECUTORCH_BUILD_PYBIND=ON EXECUTORCH_BUILD_XNNPACK=ON EXECUTORCH_BUILD_COREML=ON
$ rm -rf pip-out dist && ./install_executorch.sh --pybind xnnpack coreml

cc @larryliu0820@lucylq

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9494

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 553bbba with merge base e22b21a ():

NEW FAILURES - The following jobs have failed:

• Lint / lintrunner / linux-job (gh)
  >>> Lint for install_executorch.py:
• pull / unittest-editable / macos / macos-job (gh)
  backends/xnnpack/test/passes/test_convert_to_linear.py::TestConvertToLinear::test_fp32_convert_to_linear

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

[swolchok]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

[jathu]

This doesn't seem right to me. CMake pays attention to both environment variables and defined flags. We shouldn't have to set both. Can you give a specific motivating example?

The environment variable is not for CMake, it is used by the setup.py file we created — I included where we check for it in the description

OK, then I think the redundant part is putting the flags in CMAKE_ARGS? setup.py is already responsible for that, right?

Unfortunately, setup.py doesn't fully commit to that. For example, the reason it expects some some of these flags to be defined as an env var, is so it can easily infer if it is turned on then add additional logic like putting together pybinging. In this case, it does set the cmake flag again. It wouldn't know what environment variable should be defined as a cmake arg — we would need to make that explicit mapping. Which can just be difficult to maintain. So we should continue to let CMAKE_ARGS get set.

I guess we can remove the duplicate definition by splitting apart the CMAKE_ARGS in setup.py, and getting rid of the environment variable logic entirely. The only problem is this will now expect people to switch their workflow from:

# Before
$ EXECUTORCH_BUILD_PYBIND=ON python setup.py bdist_wheel
# After
$ CMAKE_ARGS="-DEXECUTORCH_BUILD_PYBIND=ON" python setup.py bdist_wheel

I was hesitant to change this since it might break people's current usage, and might require wider CI changes. Note that technically this diff should be neutral change because it maintains the same API — is just forces consistency of the two places of definition instead of doing it manually as it is being done now.

Thinking about it more, if we are okay with breaking people's current workflow of building wheels — I think I'd prefer to get rid of defining environment variables and just relying on CMAKE_ARGS. Thoughts?