-
Notifications
You must be signed in to change notification settings - Fork 93
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
Pull requirements/host
from pyproject.toml
#1126
base: branch-23.04
Are you sure you want to change the base?
Conversation
f72ee27
to
8dfb748
Compare
Toggling for CI |
Failures are unrelated, tracking that in #1134 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jakirkham !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakirkham, is this a pattern we plan on using for other RAPIDS libs?
I think dask-cuda
's requirements are a bit simpler than other libs (e.g. no pin_compatible
or pin_subpackage
).
So I'm wondering if this would work for more complex cases
Yeah this is something I'm looking into. Please see PR ( rapidsai/rmm#1220 ) for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily requesting changes to block this PR from being merged.
I think we need to talk more about dependency management based on @vyasr's comment below.
Seems like we have conflicting efforts in motion so we should probably sync on the best way to handle this so that we're consistent across all repositories.
Generally agree we should discuss how we handle requirements in RAPIDS more generally However with Dask-CUDA would push back on this slightly since there is a longer history here of wanting requirements in one place that is pulled into other sources ( #569 ) ( #893 ). Additionally Dask-CUDA being a pure Python package has generally been more aligned with requirements in Conda & PyPI packages. So in the Dask-CUDA case this is more a continuation of how things have been as opposed to a new approach |
The main question from the rmm PR that IMO needs to be answered before we move forward with changes like this even in dask-cuda is how we want to handle the interaction between rapids-dependency-file-generator and meta.yaml reading from pyproject.toml. Those are two different approaches for single-sourcing dependencies, and I would prefer not adding two levels of indirection (dependencies.yaml->pyproject.toml->meta.yaml) where only one might suffice if we could directly write meta.yaml from dependencies.yaml. The open questions in the rmm PR would help us understand whether there is a strong argument in favor of the two pass approach (maybe because we simply decide that supporting meta.yaml is too much trouble for dfg). |
In addition to pulling the Conda recipe's
requirements/run
frompyproject.toml
, pullrequirements/host
frompyproject.toml
. This should help solidify aroundpyproject.toml
being the single source of truth.