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

Drop workaround for glibc<2.25 #211

Draft
wants to merge 2 commits into
base: branch-0.38
Choose a base branch
from

Conversation

pentschev
Copy link
Member

Due to a glibc bug in versions prior to 2.25,
#93 implemented a workaround to the use of std::condition_variables with a spinlock. Now that RAPIDS is dropping support for CentOS 7, the workaround can also be removed.

Due to a `glibc` bug in versions prior to 2.25,
rapidsai#93 implemented a workaround to the
use of `std::condition_variable`s with a spinlock. Now that RAPIDS is
dropping support for CentOS 7, the workaround can also be removed.
@pentschev pentschev requested a review from a team as a code owner March 16, 2024 08:55
@pentschev pentschev marked this pull request as draft March 16, 2024 08:55
@pentschev
Copy link
Member Author

My initial idea was to drop this workaround and simply prevent building with glibc<2.25. Chatting offline with @bdice he made me realize this won't prevent from running with glibc<2.25, therefore we need a runtime check, similar to what we already have, to prevent users from running on older versions of glibc. I'm not sure if this helps significantly in reducing footprint for simpler maintainability, therefore I think we have to make a choice. The options I see are:

  1. Leave everything as is;
  2. Leave everything as is but add a warning in case we hit the codepath, saying it will be suboptimal given the glibc version that is running on the system;
  3. Prevent users from running code that requires the callback notifier entirely on older glibc;
  4. Prevent users from running UCXX entirely on older glibc.

I don't know what makes more sense here, perhaps either 1 or 2 would make more sense. Option 3 probably adds more code complexity than the existing workaround, since we might have to add checks in other places as well, so probably the worst option from maintainability. Option 4 is also on the table but perhaps not great, as it would limit even users who may not care about cases in which callback notifiers are needed (currently, used if the worker progress thread runs).

Do you have any opinions on this matter @wence- ?

@bdice
Copy link
Contributor

bdice commented Mar 16, 2024

I’d vote for (1), and just delete the compatibility code someday.

We can build conda packages to require the virtual package __glibc>=2.25 too, though it won’t affect wheels.

@pentschev
Copy link
Member Author

We can build conda packages to require the virtual package __glibc>=2.25 too, though it won’t affect wheels.

Although that would help specifically for conda, I'm also concerned with potential future C++ users who may build from source. So probably just keeping everything as is sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants