-
Notifications
You must be signed in to change notification settings - Fork 310
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
pylibcugraph: declare cupy and numpy hard dependencies #4854
base: branch-25.02
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@@ -3,6 +3,7 @@ | |||
[build-system] | |||
|
|||
requires = [ | |||
"numpy>=1.23,<3.0a0", |
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.
cugraph-service-server
should have a hard runtime dependency on numpy
, because of this:
import numpy as np |
its conda recipe does:
- numpy >=1.23,<3.0a0 |
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.
agreed, thanks for being so thorough!
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!
@@ -3,6 +3,7 @@ | |||
[build-system] | |||
|
|||
requires = [ | |||
"numpy>=1.23,<3.0a0", |
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.
agreed, thanks for being so thorough!
dependencies.yaml
Outdated
@@ -174,6 +178,7 @@ files: | |||
extras: | |||
table: build-system | |||
includes: | |||
- depends_on_numpy |
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.
Since numpy doesn’t have any matrices for CUDA version or any extra index URLs, I would treat it as a normal dependency and add it to the corresponding lists. We don’t use depends_on_numpy
in other repos because it doesn’t reduce complexity. You could use a YAML anchor for deduplication of the pinning declaration if desired.
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.
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.
Ok sure.
That is not how I've been thinking about when to do these depends_on_*
lists. At https://github.com/rapidsai/build-planning/blob/d9e3c606d95c835ee384ac6480a4af0ac6cb024a/docs/docs/packaging.md#L181 we have this:
Dependencies appearing in several lists should be in their own standalone
depends_on_{project}
lists.
I can update those docs with this added nuance, I don't disagree with you.
Pushed 6a83f20 here removing depends_on_numpy
in favor of anchors. That revealed that there were some other issues in cugrpah-service-server
's [test]
extra... it was pulling through all of cugraph's
testing dependencies, which it doesn't need based on my read of https://github.com/rapidsai/cugraph/tree/branch-25.02/python/cugraph-service/server/cugraph_service_server/testing
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.
Yes, I think those docs should be updated. I think it should be something like:
Dependencies appearing in several lists can be handled in two ways. Dependencies with complex requirements such as extra index URLs or package names that vary by CUDA version (like
-cuXX
suffixes) or format (different conda and PyPI names) should be in their own standalonedepends_on_{project}
lists. Simpler dependencies can use YAML anchors to avoid duplication of the pinning specs.
I'll re-review and approve based on this. Feel free to update those docs as you see fit.
While testing stuff for #4804, I found that
pylibcugraph
has a hard runtime dependency oncupy
andnumpy
, but isn't declaring themThis declares those dependencies.
It also promotes
cugraph-service-server
'snumpy
dependency to a hard runtime dependency.Notes for Reviewers
Evidence that
pylibcugraph
already has a hard dependency on these librariesThey're used unconditionally here:
cugraph/python/pylibcugraph/pylibcugraph/utils.pyx
Lines 19 to 20 in cddd69e
But have import guards in other places:
cugraph/python/pylibcugraph/pylibcugraph/sssp.pyx
Lines 127 to 139 in cddd69e
So this PR doesn't introduce new hard dependencies... it just makes them explicit, to make it easier to install and run
pylibcugraph
.How was this not caught in CI?
Import tests aren't run here for conda packages, because conda builds happen on CPU-only nodes.
cugraph/ci/build_python.sh
Lines 27 to 30 in cddd69e
And
numpy
andcupy
are probably getting pulled in by some of the wheels' test dependencies, likecudf
, here:cugraph/ci/test_wheel.sh
Line 17 in cddd69e
Should we just make the other unconditional cases conditional with try-catching?
No. Talked with @rlratzel, @ChuckHastings, and @eriknw offline, and we agreed to declare these as hard runtime dependencies (and remove the try-catching in places that had it).