-
Notifications
You must be signed in to change notification settings - Fork 21
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
Create Conda CI test env in one step #144
Create Conda CI test env in one step #144
Conversation
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.
Are ARM tests doing anything here? Maybe they should exit earlier (or be skipped in the CI matrix) if not.
There are a few comments of the form # Reactivate the test environment back
. I’m not sure if we have a test environment to go back to, with the new single-solve environments.
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.
We can do that in a follow-up PR. I'd like to limit the scope of this one to consolidating the environment creation.
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.
Left some suggestions. Leaving a blocking review, as a couple should definitely be resolved before merging this, and the question about depends_on_ogb
needs a little discussion.
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" \ | ||
--prepend-channel "${CPP_CHANNEL}" \ | ||
--prepend-channel "${PYTHON_CHANNEL}" \ | ||
--prepend-channel 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.
Not a blocking comment for this PR, but @tingyu66 @alexbarghi-nv are we planning to drop use of the pytorch
channel in the 25.04 release?
Linking this related conversation: #99 (comment)
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.
Yes, we should do that.
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.
Thank you! I can get that started in a follow-up PR if you'd like.
- depends_on_cudf | ||
- depends_on_dgl | ||
- depends_on_pytorch | ||
- depends_on_ogb |
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 don't think this is what you want, to replace conda install ogb
.
depends_on_ogb
... doesn't seem to contain ogb
:
Lines 438 to 464 in 2f8afde
# Will remove this after snap-stanford/ogb#497 is resolved. | |
# Temporarily sets the max pytorch version to 2.5 for compatibility | |
# with ogb. | |
depends_on_ogb: | |
common: | |
- output_types: [conda] | |
packages: | |
- pytorch>=2.3,<2.6a0 | |
specific: | |
- output_types: [requirements] | |
matrices: | |
- matrix: {cuda: "12.*"} | |
packages: | |
- --extra-index-url=https://download.pytorch.org/whl/cu121 | |
- matrix: {cuda: "11.*"} | |
packages: | |
- --extra-index-url=https://download.pytorch.org/whl/cu118 | |
- {matrix: null, packages: null} | |
- output_types: [requirements, pyproject] | |
matrices: | |
- matrix: {cuda: "12.*"} | |
packages: | |
- torch>=2.3,<2.6a0 | |
- matrix: {cuda: "11.*"} | |
packages: | |
- torch>=2.3,<2.6a0 | |
- {matrix: null, packages: [*pytorch_pip]} |
That looks like a mistake, not sure where it happened. @tingyu66 could you please take a look? Maybe that was the result of a bad merge conflict resolution or something. depends_on_ogb
seems to just contain torch
(which we already have depends_on_pytorch
for)
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.
It's not a mistake
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 is a temporary constraint of PyTorch that only applies to test environments where ogb is being used.
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.
Bradley and I discussed this here: #104
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 on Slack
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.
A better name for this might be ogb_pytorch_constraint
.
We typically use depends_on_
to indicate an actual dependency on just that package.
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.
Got it, thanks! The comment directly above this, about this being there for the benefit of ogb
, now makes sense to me.
It still would be better to name it ogb_pytorch_constraint
, I think, but that doesn't have to hold up this PR.
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, also devcontainer builds will fail until #148 gets merged. I expect that to pass CI now that the cudf fix is merged.
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.
Now that that's merged, I've merged in latest branch-25.04
here to re-run CI.
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.
Approving, my comments have been addressed.
I'll do the renaming suggested in #144 (comment) + dropping the pytorch
channel in a follow-up PR.
/merge |
Issue: rapidsai/build-planning#22