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

remove openmpi ceiling #4496

Merged
merged 4 commits into from
Jun 27, 2024
Merged

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Jun 24, 2024

fixes #4474

#4496 and related PRs introduced a ceiling on openmpi, a dependency that's only pulled in at test time, because cugraph's builds were struggling to find it.

This proposes removing that pin, as the fixes in conda-forge/openmpi-feedstock#159 should allow the package to again be found by e.g. find_package(MPI) in CMake scripts.

@github-actions github-actions bot added the conda label Jun 24, 2024
@jameslamb jameslamb added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 24, 2024
@jameslamb jameslamb changed the title WIP: remove openmpi pin WIP: remove openmpi pin (fixes #4474) Jun 26, 2024
@jameslamb jameslamb changed the title WIP: remove openmpi pin (fixes #4474) remove openmpi pin (fixes #4474) Jun 26, 2024
@jameslamb jameslamb marked this pull request as ready for review June 26, 2024 20:34
@jameslamb jameslamb requested a review from a team as a code owner June 26, 2024 20:34
@jameslamb jameslamb changed the title remove openmpi pin (fixes #4474) remove openmpi pin Jun 26, 2024
@jameslamb jameslamb changed the title remove openmpi pin remove openmpi ceiling Jun 26, 2024
@jameslamb
Copy link
Member Author

The unit test failures here look like they're a result of the new thriftpy2 release. I see the exact same issue as @rlratzel reported in Thriftpy/thriftpy2#281.

(build link)

I've pushed a pin here temporarily putting a ceiling on thriftpy2 in the test environment (thriftpy2<=0.5.0). Hopefully that'll unblock CI until a new thriftpy2 release with a fix is out.

@ChuckHastings
Copy link
Collaborator

/merge

@rapids-bot rapids-bot bot merged commit ddd9c19 into rapidsai:branch-24.08 Jun 27, 2024
131 checks passed
@jameslamb jameslamb deleted the remove-openmpi-pin branch June 27, 2024 16:03
@jakirkham
Copy link
Member

Thanks all! 🙏

rapids-bot bot pushed a commit that referenced this pull request Jul 19, 2024
#4496 introduced a ceiling on `thriftpy2`. Context: #4496 (comment)

The bug that ceiling was added to avoid was fixed in v0.5.2 of `thriftpy2`, which was just released (#4478 (comment)).

This removes that, adding `!=` constraints to skip the 2 versions that `cugraph` was not compatible with.

## Notes for Reviewers

### Why not a floor?

I'm proposing adding `!=` constraints to skip v0.5.0 and v0.5.1 to maximize `cugraph`'s compatibility with other environments... that'd allow it to be used in environments with `thriftpy2<0.5.0` and in environments with `thriftpy2>0.5.2`.

Let me know if you'd prefer the simplicity of a floor like `>=0.5.2` instead.

Authors:
  - James Lamb (https://github.com/jameslamb)
  - Ralph Liu (https://github.com/nv-rliu)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ralph Liu (https://github.com/nv-rliu)
  - Rick Ratzel (https://github.com/rlratzel)

URL: #4521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conda 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.

Unpin openmpi Once Build Issues are Resolved
4 participants