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

Require UCX 1.12.1+ #638

Merged
merged 1 commit into from
May 5, 2022
Merged

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham requested a review from a team as a code owner May 4, 2022 03:35
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 4, 2022
@@ -0,0 +1,2 @@
ucx_version:
- "1.12.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be >=1.12.1

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent here is to ensure we have a fixed version at build time so that this sets a minimum version at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought we wanted to build using the latest version available. What's the difference between doing this and using pin_compatible for the runtime dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are using the latest version since 1.12.1 is latest. However we may want more flexibility if a later 1.12.x breaks something and we need to pin an older version in the metapackage. Also we may want to control which version we build against to make it easier to align with other packages that have ucx constraints as well.

FWIW am proposing something similar in conda-forge ( conda-forge/conda-forge-pinning-feedstock#2847 ) though maybe we can wait to see what others think about that and update if needed.

@@ -42,6 +43,7 @@ requirements:
- libraft-headers {{ version }}
- nccl>=2.9.9
- rmm {{ minor_version }}
- ucx >={{ ucx_version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the >= here

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see comment above

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, we can probably drop this line as the feedstock has run_exports, which covers adding and pinning this.

@cjnolet
Copy link
Member

cjnolet commented May 5, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 24102b8 into rapidsai:branch-22.06 May 5, 2022
@jakirkham jakirkham deleted the ucx_1_12_1 branch May 5, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants