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

[CI] Prune unused archs from libnccl #8179

Merged
merged 3 commits into from
Aug 21, 2022
Merged

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Aug 17, 2022

This further reduces the size of the compiled binary.

@hcho3 hcho3 marked this pull request as ready for review August 17, 2022 10:07
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.

Thank you for working on this. I think this needs to be a CI specific change. People might not want XGBoost to prune the nccl on their system.

Copy link
Member

@RAMitchell RAMitchell left a comment

Choose a reason for hiding this comment

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

Agree with Jiaming that users self installing probably don't need this, and it could easily break their install process.

Is it possible to create the option PRUNE_NCCL which performs the prune then modifies the nccl library path variable in find_nccl.

CMakeLists.txt Outdated Show resolved Hide resolved
@hcho3
Copy link
Collaborator Author

hcho3 commented Aug 17, 2022

People might not want XGBoost to prune the nccl on their system.

The custom target is entirely optional, as it's not part of the all target. So you need to explicitly opt in: ninja PruneLibNccl. Users who install from the source will not be affected.

@hcho3
Copy link
Collaborator Author

hcho3 commented Aug 17, 2022

Is it possible to create the option PRUNE_NCCL which performs the prune then modifies the nccl library path variable in find_nccl.

This is hard, as you'd need to modify the nccl path as a result of building a target.

@hcho3
Copy link
Collaborator Author

hcho3 commented Aug 17, 2022

@trivialfis @RAMitchell I revised this pull request so that the changes are restricted to scripts in the CI directory tests/ci_build. Can you review again?

@RAMitchell
Copy link
Member

Thanks. I was curious if it can be done in pure cmake with a bit less hacking, here is my attempt: #8183. Not sure if it's any better.

@RAMitchell
Copy link
Member

Let's go with this version I think.

@hcho3 hcho3 merged commit 35ef8ab into dmlc:master Aug 21, 2022
@hcho3 hcho3 deleted the reduce_binary_size branch August 21, 2022 08:46
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.

3 participants