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

[Breaking] Upgrade cuDF and RMM to 0.18 nightlies; require RMM 0.18+ for RMM plugin #6510

Merged
merged 3 commits into from Dec 16, 2020

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Dec 15, 2020

Also update the RMM plugin code to account for the updated signature of the template class rmm::mr::thrust_allocator<T> (rapidsai/rmm#647).

cc @harrism @jrhemstad

@pseudotensor
Copy link
Contributor

Do things like this preserve backwards compatibility? i.e. anyone using xgboost with RMM has to use nightly (unstable) rapids 0.18? Or any rapids will work?

@hcho3
Copy link
Collaborator Author

hcho3 commented Dec 16, 2020

@pseudotensor As it stands, no, this PR breaks backward compatibility.

@pseudotensor
Copy link
Contributor

pseudotensor commented Dec 16, 2020

Seems not good idea to track nightly breaking changes. xgboost should be more stable, have deprecation stages, etc. Ya, seems easy to import rmm and check version. Would be nice if rmm had standard version . But one can do:

>>> from rmm._version import get_versions
>>> get_versions()['version']
'0.14.0'

But shouldn't do this globally at import level, like @trivialfis fixed for other imports of cudf etc. Instead can use pkg_info etc. or whatever method was used there in xgboost that followed tensorflow.

@hcho3
Copy link
Collaborator Author

hcho3 commented Dec 16, 2020

Seems not good idea to track nightly breaking changes. xgboost should be more stable, have deprecation stages, etc

@pseudotensor
I agree with you in general. However, the RMM plugin is still marked as experimental and so it is exempt from the usual deprecation policy.

@harrism For now, we will make this particular breaking change. I will file a feature request for detecting the RMM version.

@pseudotensor
Copy link
Contributor

Seems not good idea to track nightly breaking changes. xgboost should be more stable, have deprecation stages, etc

@pseudotensor
I agree with you in general. However, the RMM plugin is still marked as experimental and so it is exempt from the usual deprecation policy.

@harrism For now, we will make this particular breaking change. I will file a feature request for detecting the RMM version.

Even if experimental in xgboost it doesn't follow xgboost has to track unstable nightly versions of the experimental feature. That's your choice.

@hcho3
Copy link
Collaborator Author

hcho3 commented Dec 16, 2020

@pseudotensor

Some considerations:

  • RMM plugin is by default turned off from the usual XGBoost installation (pip install xgboost). Only the XGBoost Conda package from the rapidsai channel will use it.
  • There's the issue of not having a way to obtain the RMM version from the C++ layer. Your suggestion works in Python but not in C++. I will request for a C++ macro that encodes the version string.

So in this particular occasion, I vote for the breaking change.

@hcho3
Copy link
Collaborator Author

hcho3 commented Dec 16, 2020

@pseudotensor @harrism I created rapidsai/rmm#663 to request the version string in C++. The requested feature will help us (XGBoost devs) to handle breaking changes more gracefully in the future.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Looks good to me. I will go with the breaking change. Requiring nightly dependencies when using nightly xgboost seems fine to me.

@trivialfis
Copy link
Member

Also please remove the CI tag when squashing commits as it's touching the core of xgboost.

@hcho3
Copy link
Collaborator Author

hcho3 commented Dec 16, 2020

Got it. Let me also add a [breaking] prefix, so that later I can remind myself that RMM 0.18+ is required to enable the RMM plugin.

@hcho3 hcho3 changed the title [CI] Upgrade cuDF and RMM to 0.18 nightlies [Breaking] Upgrade cuDF and RMM to 0.18 nightlies; require RMM 0.18+ for RMM plugin Dec 16, 2020
Co-authored-by: Mark Harris <mharris@nvidia.com>
@hcho3 hcho3 merged commit bf6cfe3 into dmlc:master Dec 16, 2020
@hcho3 hcho3 deleted the upgrade_rapids branch December 16, 2020 18:07
trivialfis pushed a commit to trivialfis/xgboost that referenced this pull request Feb 10, 2021
…for RMM plugin (dmlc#6510)

* [CI] Upgrade cuDF and RMM to 0.18 nightlies

* Modify RMM plugin to be compatible with RMM 0.18

* Update src/common/device_helpers.cuh

Co-authored-by: Mark Harris <mharris@nvidia.com>

Co-authored-by: Mark Harris <mharris@nvidia.com>
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.

None yet

4 participants