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

[CI/Docker] Switch to pytorch new extra index url notation #26072

Merged
merged 10 commits into from
Jan 18, 2023
Merged

[CI/Docker] Switch to pytorch new extra index url notation #26072

merged 10 commits into from
Jan 18, 2023

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Jun 24, 2022 •
edited
Loading

Why are these changes needed?

torch now provides custom index urls, eliminating the need for the plus-notation and making reqs more readable and maintainable:

Related issue number

followup on #23580

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@ddelange ddelange changed the title Pytorch extra index url Switch to pytorch new extra index url notation Jun 24, 2022
@ddelange ddelange force-pushed the pytorch-extra-index-url branch 2 times, most recently from 21cce50 to dc02eef Compare June 26, 2022 10:54
@ddelange ddelange force-pushed the pytorch-extra-index-url branch 2 times, most recently from 7c99e81 to 82c0ac6 Compare July 6, 2022 07:29
@DmitriGekhtman DmitriGekhtman removed their request for review July 6, 2022 08:02
@wuisawesome wuisawesome removed their request for review July 7, 2022 00:44
@ddelange ddelange force-pushed the pytorch-extra-index-url branch 4 times, most recently from 48a36bf to 9f07440 Compare July 15, 2022 08:20
@ddelange ddelange force-pushed the pytorch-extra-index-url branch 3 times, most recently from 89a48db to c113963 Compare July 25, 2022 07:09
@ddelange
Copy link
Contributor Author

@amogkam CI is green :)

@amogkam amogkam changed the title Switch to pytorch new extra index url notation [CI/Docker] Switch to pytorch new extra index url notation Jul 25, 2022
amogkam
amogkam requested changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@amogkam amogkam left a comment

Choose a reason for hiding this comment

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

Thanks @ddelange! Left a couple comments

python/requirements_ml_docker.txt Outdated Show resolved Hide resolved
python/requirements_ml_docker.txt Outdated Show resolved Hide resolved
@ddelange ddelange force-pushed the pytorch-extra-index-url branch 2 times, most recently from 8b22470 to 8e8235d Compare July 25, 2022 19:27
@ddelange
Copy link
Contributor Author

@krfricke rebased on top of your PR #28808 🤙 also, pulls are out that will solve the blocking issue for upcoming (patch) releases of the pyg libs

@ddelange
Copy link
Contributor Author

@amogkam @krfricke pyg-team/pyg-lib#66 was merged 🎉

rebased on latest master, ready for review!

@ddelange
Switch to pytorch new extra index url notation
98597f3 
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange
Copy link
Contributor Author

@amogkam could you help re-running some tests? couldnt find failures related to this PR

@ddelange
Copy link
Contributor Author

cc @krfricke could you help re-run failed tests?

@ddelange
Copy link
Contributor Author

@amogkam @krfricke looks like CI is green 😍 could you have a look?

@amogkam
Copy link
Contributor

amogkam commented Jan 13, 2023

Thanks a lot @ddelange!

There is just one last issue with formatting:
python/ray/train/examples/pytorch_geometric/distributed_sage_example.py:1:89: E501 line too long (114 > 88 characters)

@ddelange
Lint
0d6e75f 
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange
Copy link
Contributor Author

@amogkam fixed 🤙

@ddelange
Copy link
Contributor Author

hi @amogkam 👋 could you rerun the failing test?

@ddelange
Copy link
Contributor Author

Hi @amogkam thanks for re-running. it looks like the same flakey failure? https://buildkite.com/ray-project/oss-ci-build-pr/builds/9440#0185b4be-d23f-4de4-acac-4ecf08a921b6

@amogkam amogkam merged commit c0f1196 into ray-project:master Jan 18, 2023
@ddelange ddelange mentioned this pull request Jan 18, 2023
Closed
andreapiso pushed a commit to andreapiso/ray that referenced this pull request Jan 22, 2023
@ddelange @andreapiso
[CI/Docker] Switch to pytorch new extra index url notation (ray-proje…
df4817e 
…ct#26072)

torch now provides custom index urls, eliminating the need for the plus-notation and making reqs more readable and maintainable

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
@ddelange ddelange mentioned this pull request Feb 19, 2023
Open
@ddelange ddelange mentioned this pull request Sep 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amogkam amogkam amogkam approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977

@richardliaw richardliaw Awaiting requested review from richardliaw

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng is a code owner

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

Assignees

@amogkam amogkam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ddelange @amogkam