Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add PyYaml to requirements.txt #1174

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Add PyYaml to requirements.txt #1174

merged 1 commit into from
Aug 11, 2022

Conversation

rengolin
Copy link
Member

@rengolin rengolin commented Aug 8, 2022

Building on a fresh environment + virtualenv + in-tree build errors out
becayse PyYaml isn't installed. Adding to requirements.txt fixes that.

Fixes #1173

@rengolin
Copy link
Member Author

rengolin commented Aug 8, 2022

I'm guessing that was a spurious failure?

@rengolin
Copy link
Member Author

rengolin commented Aug 8, 2022

Hm, weird, I just created another env and PyYaml was installed automatically... Maybe I missed something on the previous attempt. Ignore this.

@rengolin rengolin closed this Aug 8, 2022
@rengolin rengolin deleted the pyyaml branch August 8, 2022 12:30
@rengolin rengolin restored the pyyaml branch August 10, 2022 15:39
@rengolin rengolin reopened this Aug 10, 2022
@rengolin
Copy link
Member Author

Right now, I'm having to work around like this.

@powderluv
Copy link
Collaborator

do we know what error'ed out ? Curious where that dep comes from

@rengolin
Copy link
Member Author 

FAILED: tools/torch-mlir/generated_backend.hash tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/LazyNativeFunctions.cpp tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/RegisterLazy.cpp tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/shape_inference.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/generated_backend.hash /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/LazyNativeFunctions.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/RegisterLazy.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/shape_inference.cpp 
cd /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend && /home/rengolin/devel/mlir/torch-mlir/mlir_venv/bin/python3.10 /home/rengolin/devel/mlir/torch-mlir/build_tools/autogen_ltc_backend.py -b /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir
Traceback (most recent call last):
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/utils.py", line 34, in <module>
    from yaml import CSafeLoader as Loader
ModuleNotFoundError: No module named 'yaml'

@rengolin
Copy link
Member Author 

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/rengolin/devel/mlir/torch-mlir/build_tools/autogen_ltc_backend.py", line 17, in <module>
    import torchgen.dest.lazy_ir
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/dest/__init__.py", line 1, in <module>
    from .lazy_ir import (
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/dest/lazy_ir.py", line 6, in <module>
    import torchgen.api.dispatcher as dispatcher
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/api/dispatcher.py", line 4, in <module>
    from torchgen.api import cpp
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/api/cpp.py", line 4, in <module>
    from torchgen.api.types import (
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/api/types.py", line 5, in <module>
    from torchgen.model import (
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/model.py", line 9, in <module>
    from torchgen.utils import assert_never, NamespaceHelper, OrderedSet
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/utils.py", line 36, in <module>
    from yaml import SafeLoader as Loader  # type: ignore[misc]
ModuleNotFoundError: No module named 'yaml'

@powderluv
Copy link
Collaborator

Ahh LTC dependency. For now you can disable LTC too with -DTORCH_MLIR_ENABLE_LTC=OFF

@henrytwo @antoniojkim

@rengolin
Copy link
Member Author

rengolin commented Aug 10, 2022 •
edited
Loading

Is this something that would go to the dev doc?

How do you map the packages from requirements.txt and the build-time choices from CMake?

I always assumed you'd put all deps in the file, especially one used by a default build option.

@antoniojkim
Copy link
Collaborator 

FAILED: tools/torch-mlir/generated_backend.hash tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/LazyNativeFunctions.cpp tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/RegisterLazy.cpp tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/shape_inference.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/generated_backend.hash /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/LazyNativeFunctions.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/RegisterLazy.cpp /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend/generated/shape_inference.cpp 
cd /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir/python/torch_mlir/csrc/base_lazy_backend && /home/rengolin/devel/mlir/torch-mlir/mlir_venv/bin/python3.10 /home/rengolin/devel/mlir/torch-mlir/build_tools/autogen_ltc_backend.py -b /home/rengolin/devel/mlir/torch-mlir/build/tools/torch-mlir
Traceback (most recent call last):
  File "/home/rengolin/devel/mlir/torch-mlir/mlir_venv/lib/python3.10/site-packages/torchgen/utils.py", line 34, in <module>
    from yaml import CSafeLoader as Loader
ModuleNotFoundError: No module named 'yaml'

hmm, this is weird. yaml should be dependency of PyTorch and should thus get pulled in automatically when we install the PyTorch wheel.

@rengolin
Copy link
Member Author

What got me confused is that, when I ran this step:

python setup.py bdist_wheel

The deps got pulled and I could build, but I had to clean the build directory, otherwise I get a CMake error that it isn't the same CMakeLists.txt file.

When I don't run that step, the PyPaml dep isn't pulled, but I can build straight away without building the bdist_wheel (if I pip install pyyaml before).

But those are different builds and one shouldn't have to depend on the other, nor should I build it twice, just to get the PyYaml dep.

@antoniojkim
Copy link
Collaborator

What got me confused is that, when I ran this step:

python setup.py bdist_wheel

The deps got pulled and I could build, but I had to clean the build directory, otherwise I get a CMake error that it isn't the same CMakeLists.txt file.

When I don't run that step, the PyPaml dep isn't pulled, but I can build straight away without building the bdist_wheel (if I pip install pyyaml before).

But those are different builds and one shouldn't have to depend on the other, nor should I build it twice, just to get the PyYaml dep.

ah, I see. I think what I do is set the environment variable TORCH_MLIR_CMAKE_BUILD_DIR_ALREADY_BUILT=1 when building the wheel so that the cmake stuff doesn't get run again.

@rengolin
Copy link
Member Author

ah, I see. I think what I do is set the environment variable TORCH_MLIR_CMAKE_BUILD_DIR_ALREADY_BUILT=1 when building the wheel so that the cmake stuff doesn't get run again.

Right, but this shouldn't change the installation of PyYaml before the main build.

What is the downside of adding PyYaml to requirements.txt? The build is using it with its default CMake options, and it doesn't get installed before the main build. I think adding more steps in the developer documentation is a worse fix than just adding to the requirement list.

@silvasean
Copy link
Contributor

hmm, this is weird. yaml should be dependency of PyTorch and should thus get pulled in automatically when we install the PyTorch wheel.

It seems that torch doesn't declare yaml as a dependency.

$ pipdeptree | grep torch -A3
torchvision==0.14.0.dev20220805+cpu
  - numpy [required: Any, installed: 1.23.1]
  - pillow [required: >=5.3.0,!=8.3.*, installed: 9.2.0]
  - requests [required: Any, installed: 2.27.1]
  - torch [required: ==1.13.0.dev20220805+cpu, installed: 1.13.0.dev20220805+cpu]
    - typing-extensions [required: Any, installed: 4.2.0]
  - typing-extensions [required: Any, installed: 4.2.0]

I think we should add PyYAML to requirements.txt for now as a workaround. @antoniojkim can you dig into why torch/torchgen doesn't declare PyYAML as a dependency and file a bug on their side or us to link to?

silvasean
silvasean approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment saying something like "workaround for ..., see discussion in #1174 for more details"?

@powderluv
Copy link
Collaborator

It seems like they have a build time https://github.com/pytorch/pytorch/blob/beceb8b92f030b19a0ea48ab9ae0def8f554e7f3/requirements.txt#L7 and runtime dep https://github.com/pytorch/pytorch/blob/beceb8b92f030b19a0ea48ab9ae0def8f554e7f3/pyproject.toml#L8

@silvasean
Copy link
Contributor

It seems like they have a build time https://github.com/pytorch/pytorch/blob/beceb8b92f030b19a0ea48ab9ae0def8f554e7f3/requirements.txt#L7 and runtime dep https://github.com/pytorch/pytorch/blob/beceb8b92f030b19a0ea48ab9ae0def8f554e7f3/pyproject.toml#L8

So the problem is that we aren't installing their build deps even though we are using their build system. In other words, we don't really depend on the torch package, but also the build system too. @stellaraccident do you know if there is a way to install their build deps too?

@antoniojkim
Copy link
Collaborator

@antoniojkim can you dig into why torch/torchgen doesn't declare PyYAML as a dependency and file a bug on their side or us to link to?

pytorch/pytorch#83257

@antoniojkim antoniojkim mentioned this pull request Aug 11, 2022
Open
@rengolin
Add PyYaml to requirements.txt
436b192 
Building on a fresh environment + virtualenv + in-tree build errors out
becayse PyYaml isn't installed. Adding to requirements.txt fixes that.

Fixes llvm#1173
@rengolin rengolin merged commit 51bfe25 into llvm:main Aug 11, 2022
@tanyokwok tanyokwok mentioned this pull request Sep 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvasean silvasean silvasean approved these changes

@powderluv powderluv Awaiting requested review from powderluv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ModuleNotFoundError: No module named 'yaml'
4 participants
@rengolin @powderluv @antoniojkim @silvasean