-
Notifications
You must be signed in to change notification settings - Fork 326
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.
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
[CI] Fix nightly build dependency on tensordict #1300
Conversation
setup.py
Outdated
if is_nightly: | ||
return "torch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skandermoalla I was wondering if we could instruct to install torch nightly (which might be the reason of your problem) but i can't really find how to tell install-requires to use the pip command provided by pytorch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I was able to build and use a TorchRL wheel by cloning the repo and running python setup.py bdist_wheel
which uses the local environment (the one I listed) instead of building a sandboxed one (from the deps in the setup). I'll add this comment to the issue for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I just noticed that the torchrl-nightly
was built in an environment using torch-nightly
.
I thought it was using the build dependencies listed in pyproject.toml
.
So as you said that's probably why it doesn't work in an environment with non-nightly PyTorch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could put torch>2.1
for the nightlies, which will break with anyone who does not have the nightlies.
I'm not sure how to capture the resulting error and point users in the right direction though as I would imagine that it will simply tell you that this version is not available...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this (https://stackoverflow.com/questions/24443583/using-an-extra-python-package-index-url-with-setup-py) but it seems deprecated and I'm looking for the equivalent using wheel
which provides bdist_wheel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some documentation here (https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#direct-url-dependencies).
install_requires=[
"torch @ https://download.pytorch.org/whl/nightly/cpu/torch-2.1.0.dev20230620-cp39-none-macosx_11_0_arm64.whl",
...
Is the closest I could get, but you have to specify the full path to the wheel (here torch-2.1.0.dev20230620-cp39-none-macosx_11_0_arm64.whl
) which is not convenient.
Some of the tags could be recovered programmatically using environment markers I'm not sure we can recover everything.
I don't think it's possible to do otherwise :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's try with torch 2.1 then, at least there will be an exception thrown
I will have a look at whether we can capture it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be working, if you want to check.
If you have torch 2.1.0dev installed it will work ok, but if you have not the nightly you'll get
ERROR: Could not find a version that satisfies the requirement torch>=2.1.0.dev (from torchrl-nightly) (from versions: 1.7.1, 1.8.0, 1.8.1, 1.9.0, 1.9.1, 1.10.0, 1.10.1, 1.10.2, 1.11.0, 1.12.0, 1.12.1, 1.13.0, 1.13.1, 2.0.0, 2.0.1)
ERROR: No matching distribution found for torch>=2.1.0.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested. Works as expected!
Fixes #1297
cc @skandermoalla