-
Notifications
You must be signed in to change notification settings - Fork 13
[REVIEW] Minor changes to conda recipe #47
base: branch-0.8
Are you sure you want to change the base?
Conversation
1. Because dask-cudf has the dependencies like dask, distributed, python and cudf, we can remove them as direct dependencies of dask-cuml. 2. 'test' section can check import of dask_cuml instead of some of its dependencies.
Would someone please review this PR ? Thanks. |
Hi @dantegd, Would you please review ? Thanks. |
@@ -32,27 +32,17 @@ build: | |||
|
|||
requirements: | |||
build: | |||
- python | |||
- cudf 0.7* | |||
- dask >=0.19.0 |
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.
We should keep python
here.
- dask-cudf 0.7* | ||
- dask-cuda 0.7* | ||
run: | ||
- python | ||
- cudf 0.7* | ||
- dask >=0.19.0 |
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.
Would also keep python
and dask
here.
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.
I thought the indirect dependency of python(or dask) through dask-cudf is good enough. Any specific reason why we still want to add them explicitly ?
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.
It's best practice to list explicit requirements. We are using dask
in this library explicitly.
Also we need to list everything in run
that we expect to be installed with this package. That includes python
.
python and cudf, we can remove them as direct dependencies of
dask-cuml.
its dependencies.