search

rapidsai / ucx-wheels

BSD 3-Clause "New" or "Revised" License
1 stars 4 forks source link

prefer wheel-provided libraries, use RTLD_LOCAL #13

Closed jameslamb closed 4 days ago

jameslamb commented 2 weeks ago

Description

Contributes to https://github.com/rapidsai/build-planning/issues/118

Modifies libucx.load_library() in the following ways:

Also updates all the pre-commit hook versions while I'm touching this repo. That was harmless, and I especially wanted to get up to the latest version of the RAPIDS copyright hook.

Notes for Reviewers

Version changes?

Proposing starting with 1.15.0.post2 because it's the oldest version supported at build and runtime by ucxx (code link).

And doing the following, in this order:

  1. merge this PR
  2. in a ucxx PR, test with these packages
  3. publish 1.15.0.post2 packages (via manually triggering CI run here)
  4. put up PRs to publish each of the other versions (https://pypi.org/project/libucx-cu12/#history)
    • 1.14.1.post2
    • 1.16.0.post2
    • 1.17.0.post1 (there was never a 1.17.0.post1)
jameslamb commented 2 weeks ago

@pentschev for why I'm proposing rebuilding 1.14 even though RAPIDS doesn't support it... same as the last time we talked about that: https://github.com/rapidsai/ucx-wheels/pull/6#issuecomment-2104979402

It's not that much incremental effort, and it ensures all the wheels work the same way regardless of which version you get. I don't feel too strongly about that, just proposing it because it's what we did the last time we had an update like this.

pentschev commented 1 week ago

for why I'm proposing rebuilding 1.14 even though RAPIDS doesn't support it... same as the last time we talked about that: #6 (comment)

It's not that much incremental effort, and it ensures all the wheels work the same way regardless of which version you get. I don't feel too strongly about that, just proposing it because it's what we did the last time we had an update like this.

I see, I would say I understand but I don't know if it's a great idea to keep on changing older versions. For instance, we may break things we now don't test anymore and that were previously working when we tested them. Similar to what we do with RAPIDS, I think it's probably ok to accept the past state of potentially broken libraries. IOW, I think we're fine doing it this once but I'm less convinced we should keep on doing that in the future, open to divergent opinions though.

vyasr commented 1 week ago

I'm inclined to agree with Peter here. After this change I suggest that we stop pushing fixes for older versions than we support in RAPIDS until someone specifically asks for them.

vyasr commented 1 week ago

@trxcllnt if everything looks fine to you here feel free to merge.

jameslamb commented 4 days ago

Thanks!

@vyasr I can handle repeating this for the other versions:

jameslamb commented 3 days ago

Tested with a ucxx PR and confirmed that its CI passes with these changes: https://github.com/rapidsai/ucxx/pull/334#issuecomment-2501557083

So I think we're good to proceed.