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.

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

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Conversation

vmoens
Copy link
Contributor

@vmoens vmoens commented Jun 20, 2023

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 20, 2023
setup.py Outdated
Comment on lines 73 to 74
if is_nightly:
return "torch"
Copy link
Contributor Author

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...

Copy link
Contributor

@skandermoalla skandermoalla Jun 20, 2023

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.

Copy link
Contributor

@skandermoalla skandermoalla Jun 20, 2023

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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

Copy link
Contributor

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 :(

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

Copy link
Contributor

@skandermoalla skandermoalla left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@vmoens vmoens added the CI Has to do with CI setup (e.g. wheels & builds, tests...) label Jun 20, 2023
Copy link
Contributor

@skandermoalla skandermoalla left a 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!

@vmoens vmoens merged commit 63fb59b into main Jun 20, 2023
@vmoens vmoens deleted the fix_nightly_deps branch June 20, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Has to do with CI setup (e.g. wheels & builds, tests...) CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Nightly wheel fails at importing although non-nightly one works fine.
3 participants