torch-mlir icon indicating copy to clipboard operation
torch-mlir copied to clipboard

Default build dir in script matches the one in setup.py

Open penguin-wwy opened this issue 1 year ago • 3 comments

Default build dir in setup.py https://github.com/llvm/torch-mlir/blob/5ecc1d5c0dbb8ac166c36a083c982eb27e6f33eb/setup.py#L65-L66 Keep them in line.

penguin-wwy avatar Mar 13 '24 11:03 penguin-wwy

Yes, I initially followed the development.md to complete the build. When packaging torch_mlir as a dependency using the setup.py build command, I noticed some inconsistencies with the development.md behavior, including:

  • Does not automatically execute the script(e.g. update_torch_ods.sh)
  • The default build directory used is not consistent, and I need to specify it using TORCH_MLIR_BUILD_DIR.

Perhaps I can try modifying the setup.py to make them consistent. Including automatically executing source generation, to achieve a one-click package.

penguin-wwy avatar Mar 13 '24 12:03 penguin-wwy

Best would be if we can make setuptools not steal the build directory name. While perhaps a minor thing, the mlir c++ developers are quite used to having build/ be for cmake. It would be nice if pip moved and they could both exist in their preferred way.

Possibility: https://stackoverflow.com/questions/66163088/how-to-change-build-directory-in-setuptools

stellaraccident avatar Mar 13 '24 13:03 stellaraccident

Best would be if we can make setuptools not steal the build directory name. While perhaps a minor thing, the mlir c++ developers are quite used to having build/ be for cmake. It would be nice if pip moved and they could both exist in their preferred way.

Possibility: https://stackoverflow.com/questions/66163088/how-to-change-build-directory-in-setuptools I opened a new PR to implement this feature (and made some enhancements to setuptools), because it was inconsistent with the original intention of my PR.

Using a new build directory also introduces a new problem: the default build path in update_torch_ods.sh cannot matches the one in setup.py. This means that after building with setup.py, must specify TORCH_MLIR_BUILD_DIR to execute update_torch_ods.sh.

I think there are two ways to solve this:

  • Setuptools uses a new directory for packaging the wheel, but still uses build/ when calling cmake. We can check the cmake cache config to determine if an update is needed.
  • Setuptools is only used for wheel packaging, avoiding operations like setup.py develop/build and documenting this in the documentation.

penguin-wwy avatar Mar 13 '24 16:03 penguin-wwy

Best would be if we can make setuptools not steal the build directory name. While perhaps a minor thing, the mlir c++ developers are quite used to having build/ be for cmake. It would be nice if pip moved and they could both exist in their preferred way. Possibility: https://stackoverflow.com/questions/66163088/how-to-change-build-directory-in-setuptools I opened a new PR to implement this feature (and made some enhancements to setuptools), because it was inconsistent with the original intention of my PR.

Using a new build directory also introduces a new problem: the default build path in update_torch_ods.sh cannot matches the one in setup.py. This means that after building with setup.py, must specify TORCH_MLIR_BUILD_DIR to execute update_torch_ods.sh.

I think there are two ways to solve this:

  • Setuptools uses a new directory for packaging the wheel, but still uses build/ when calling cmake. We can check the cmake cache config to determine if an update is needed.
  • Setuptools is only used for wheel packaging, avoiding operations like setup.py develop/build and documenting this in the documentation.

I think I prefer this second option, but I know it is annoying.

stellaraccident avatar Apr 01 '24 21:04 stellaraccident

Best would be if we can make setuptools not steal the build directory name. While perhaps a minor thing, the mlir c++ developers are quite used to having build/ be for cmake. It would be nice if pip moved and they could both exist in their preferred way. Possibility: https://stackoverflow.com/questions/66163088/how-to-change-build-directory-in-setuptools I opened a new PR to implement this feature (and made some enhancements to setuptools), because it was inconsistent with the original intention of my PR.

Using a new build directory also introduces a new problem: the default build path in update_torch_ods.sh cannot matches the one in setup.py. This means that after building with setup.py, must specify TORCH_MLIR_BUILD_DIR to execute update_torch_ods.sh. I think there are two ways to solve this:

  • Setuptools uses a new directory for packaging the wheel, but still uses build/ when calling cmake. We can check the cmake cache config to determine if an update is needed.
  • Setuptools is only used for wheel packaging, avoiding operations like setup.py develop/build and documenting this in the documentation.

I think I prefer this second option, but I know it is annoying.

If go with the second option, I will try to make the python setup.py develop/build command throws an error message, prompting the user that the setup.py is only used for packaging the wheel. I will also update the development doc. In addition, I plan to revise some outdated content in the development documentation, which may take a considerable amount of time.

penguin-wwy avatar Apr 02 '24 07:04 penguin-wwy

Thank you. If you see a way to simplify while doing it, let me know.

In my opinion, we should simply delete development docs that are wrong. I'd rather have people have to ask than sending down the wrong path. Obviously, full/good docs would be better: I'm just saying that if you don't have the time to update them all, we can just delete for now.

stellaraccident avatar Apr 02 '24 16:04 stellaraccident

Best would be if we can make setuptools not steal the build directory name. While perhaps a minor thing, the mlir c++ developers are quite used to having build/ be for cmake. It would be nice if pip moved and they could both exist in their preferred way. Possibility: https://stackoverflow.com/questions/66163088/how-to-change-build-directory-in-setuptools I opened a new PR to implement this feature (and made some enhancements to setuptools), because it was inconsistent with the original intention of my PR.

Using a new build directory also introduces a new problem: the default build path in update_torch_ods.sh cannot matches the one in setup.py. This means that after building with setup.py, must specify TORCH_MLIR_BUILD_DIR to execute update_torch_ods.sh. I think there are two ways to solve this:

  • Setuptools uses a new directory for packaging the wheel, but still uses build/ when calling cmake. We can check the cmake cache config to determine if an update is needed.
  • Setuptools is only used for wheel packaging, avoiding operations like setup.py develop/build and documenting this in the documentation.

I think I prefer this second option, but I know it is annoying.

If go with the second option, I will try to make the python setup.py develop/build command throws an error message, prompting the user that the setup.py is only used for packaging the wheel. I will also update the development doc. In addition, I plan to revise some outdated content in the development documentation, which may take a considerable amount of time.

I completed these tasks in a new PR #3162 . @stellaraccident

penguin-wwy avatar Apr 14 '24 17:04 penguin-wwy