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.

Sign up for GitHub

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

Jump to bottom

[WIP] Use one metapackage for CPU/GPU #35

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[WIP] Use one metapackage for CPU/GPU #35

wants to merge 7 commits into from

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Oct 25, 2019 •
edited

Based on discussion in issue ( #23 ), it seems that CPU/GPU selection matters mainly for package size and only really affects the C++ library. Given this information, this simplifies the recipe to have one metapackage to select between CPU/GPU builds. Note that we have not added GPU builds in this PR, but hopefully this should simplify the process of adding such a GPU PR in the near future.

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • It looks like the 'libxgboost' output doesn't have any tests.
  • It looks like the 'xgboost' output doesn't have any tests.

@jakirkham jakirkham mentioned this pull request Oct 25, 2019
Open
@jakirkham jakirkham changed the title Use one metapackage for CPU/GPU WIP: Use one metapackage for CPU/GPU Oct 25, 2019
@jakirkham jakirkham force-pushed the use_one_mutex branch 2 times, most recently from b908087 to 56727fb Compare October 28, 2019 21:18
@jakirkham jakirkham changed the title WIP: Use one metapackage for CPU/GPU Use one metapackage for CPU/GPU Oct 28, 2019
@jakirkham
Copy link
Member Author

This is ready for review! 😄

@jakirkham
Drop per package CPU/GPU mutexes
69606c3 
Based on discussions with the xgboost developers, it appears that only
the C++ library changes based on whether it is built in CPU or GPU mode.
The other language bindings do not change based on whether this
functionality exists or not. So drop all of the per package mutexes to
streamline things a bit.
@jakirkham
MNT: Re-rendered with conda-build 3.18.10, conda-smithy 3.6.0, and co…
19c435d 
…nda-forge-pinning 2019.10.22

Now that the per package CPU/GPU mutexes have been dropped, re-render
the feedstock to update its content accordingly.
@jakirkham
Add xgboost-proc to allow CPU/GPU selection
236c6c3 
As only the C++ library changes between CPU and GPU builds, add a meta
package to allow selection between the two builds of the C++ library.
Since this does not include GPU builds yet, simply set the selection
package to CPU only.
@jakirkham
MNT: Re-rendered with conda-build 3.18.10, conda-smithy 3.6.0, and co…
dbccc17 
…nda-forge-pinning 2019.10.22

Now that `xgboost-proc` has been added for CPU/GPU selection, re-render
to update the feedstock content accordingly.
@jakirkham
Bump build number to 5
07b7095 
Rebuild all of the packages now that the CPU/GPU selection metapackages
have been restructured.
@jjhelmus
Copy link
Contributor

This setup will create a mutex for the cpu and gpu variants for xgboost but lacks a method for establishing a order between these variants. The selector/mutex package can also establish a default package (which one is a matter of debate) either by the version number or by attaching track_features to the lower priority variants.

Additionally, dropping the -cpu metapackage packages may not be ideal. Some downstream packages or user may already be using these to specify a particular variant as a requirement or in an environment. Additionally, these metapackage will show up when a user searches using conda search py-xgboost*.

One might possibly renaming the mutex from xgboost-proc to _xgboost-proc. Other packages of these type use a leading underscore to indicate that these are metapackages or selectors.

@xhochy
Copy link
Member

xhochy commented Feb 15, 2022

@jakirkham Should we revamp this one now?

@jakirkham
Copy link
Member Author

We could do that. Is there still interest in this change?

@jakirkham jakirkham mentioned this pull request Aug 31, 2022
Open
@jakirkham
Copy link
Member Author

@conda-forge-admin, please re-render

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like I wasn't able to push to the use_one_mutex branch of jakirkham-feedstocks/xgboost-feedstock. Did you check the "Allow edits from maintainers" box?

NOTE: PRs from organization accounts or PRs from forks made from organization forks cannot be rerendered because of GitHub permissions. Please fork the feedstock directly from conda-forge into your personal GitHub account.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/xgboost-feedstock/actions/runs/5473614584.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • It looks like the 'libxgboost' output doesn't have any tests.
  • It looks like the 'xgboost' output doesn't have any tests.

@jakirkham jakirkham marked this pull request as draft July 6, 2023 09:22
@jakirkham jakirkham changed the title Use one metapackage for CPU/GPU [WIP] Use one metapackage for CPU/GPU Jul 6, 2023
@jakirkham jakirkham mentioned this pull request Jul 6, 2023
Closed
@jakirkham jakirkham mentioned this pull request Aug 19, 2023
Closed
1 task
jakirkham added a commit to jakirkham-feedstocks/xgboost-feedstock that referenced this pull request Oct 14, 2023
@jakirkham
Merge pull request conda-forge#35 from jakirkham-feedstocks/merge_ups…
512c0b6 
…tream_6

Merge `conda-forge/main` into `rapidsai/main` (pt. 6)
@jakirkham jakirkham mentioned this pull request Oct 20, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldanor aldanor Awaiting requested review from aldanor aldanor is a code owner

@beckermr beckermr Awaiting requested review from beckermr

@jjhelmus jjhelmus Awaiting requested review from jjhelmus

@mingwandroid mingwandroid Awaiting requested review from mingwandroid

@fhoehle fhoehle Awaiting requested review from fhoehle fhoehle is a code owner

@ksangeek ksangeek Awaiting requested review from ksangeek ksangeek is a code owner

@xhochy xhochy Awaiting requested review from xhochy xhochy is a code owner

@hcho3 hcho3 Awaiting requested review from hcho3 hcho3 will be requested when the pull request is marked ready for review hcho3 is a code owner

@mfansler mfansler Awaiting requested review from mfansler mfansler will be requested when the pull request is marked ready for review mfansler is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jakirkham @conda-forge-linter @jjhelmus @xhochy