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

Remove NumPy <2 pin #6031

Merged
merged 12 commits into from
Aug 28, 2024
Merged

Remove NumPy <2 pin #6031

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
dff4094
Remove NumPy <2 pin.
seberg Aug 19, 2024
c781416
Include pre-release identifier for upper pin
seberg Aug 20, 2024
11b0689
Merge branch 'branch-24.10' into my_new_branch
jakirkham Aug 24, 2024
85cd885
Merge branch 'branch-24.10' into my_new_branch
jakirkham Aug 26, 2024
8e2a41b
Drop unused NumPy headers
jakirkham Aug 28, 2024
0f00c86
Refresh copyright header
jakirkham Aug 28, 2024
6243af8
Explicitly add NumPy requirement
jakirkham Aug 28, 2024
7ca536c
skip dask pytests
divyegala Aug 28, 2024
2edf876
run tests in cu11
divyegala Aug 28, 2024
f5e5882
Fix if syntax
raydouglass Aug 28, 2024
b00c929
Merge divyegala/bug-wheels_dask_pytest into seberg/my_new_branch
jakirkham Aug 28, 2024
16d2c49
Update ci/test_wheel.sh
jameslamb Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conda/environments/all_cuda-118_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ dependencies:
- pip
- pydata-sphinx-theme!=0.14.2
- pylibraft==24.10.*,>=0.0.0a0
- pynndescent==0.5.8
- pynndescent
- pytest-benchmark
- pytest-cases
- pytest-cov
Expand Down
2 changes: 1 addition & 1 deletion conda/environments/all_cuda-125_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ dependencies:
- pip
- pydata-sphinx-theme!=0.14.2
- pylibraft==24.10.*,>=0.0.0a0
- pynndescent==0.5.8
- pynndescent
- pytest-benchmark
- pytest-cases
- pytest-cov
Expand Down
2 changes: 1 addition & 1 deletion conda/recipes/cuml-cpu/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ requirements:
- rapids-build-backend>=0.3.0,<0.4.0.dev0
run:
- python x.x
- numpy>=1.23,<2.0a0
- numpy>=1.23,<3.0a0
- pandas
- scikit-learn=1.2
- hdbscan>=0.8.38,<0.8.39
Expand Down
2 changes: 1 addition & 1 deletion dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ dependencies:
- *scikit_learn
- statsmodels
- umap-learn==0.5.3
- pynndescent==0.5.8
- pynndescent
Copy link
Member

Choose a reason for hiding this comment

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

@dantegd , is it alright if we relax this pin?

Copy link
Member

Choose a reason for hiding this comment

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

Given passing tests, it should be fine to relax now

- output_types: conda
packages:
- pip
Expand Down
4 changes: 0 additions & 4 deletions python/cuml/cuml/neighbors/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,3 @@ rapids_cython_create_modules(
MODULE_PREFIX neighbors_
ASSOCIATED_TARGETS cuml
)

foreach(target IN LISTS targets_using_numpy)
target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")
endforeach()
Comment on lines -37 to -40
Copy link
Member

Choose a reason for hiding this comment

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

Trying dropping this. AFAICT the Cython modules above don't cimport numpy. So they wouldn't need this

Not sure whether it would cause the tests to hang. At a minimum, it is unused; so, it is worth cleaning up

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not sure what happened there. I traced back in the blame to where this was added. Looks like @vyasr recommended removing it back at the source: #4818 (comment)

But there wasn't any additional discussion on that PR (maybe it happened somewhere else), and the change was merged in.

I agree with you that it seems to be unused.

Copy link
Member

Choose a reason for hiding this comment

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

Originally was looking for clues on fixing the hanging test ( #6031 (comment) ). Tried this just in case it helped, but it didn't matter

Read through the history here yesterday. It seems like NumPy was a build dependency a while back (though still wasn't clear then whether it was being used). Think since then every update has assumed NumPy was a build dependency. However as we don't require it during the build, it isn't actually satisfied.

Further we would have needed find_package(Python REQUIRED COMPONENTS Development NumPy) to find NumPy and set $Python_NumPy_INCLUDE_DIRS, but we don't do that either.

Think this hasn't presented much of an issue as we don't actually set targets_using_numpy.

In any event, this time seems as good as any for cleaning this up

2 changes: 1 addition & 1 deletion python/cuml/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ test = [
"hypothesis>=6.0,<7",
"nltk",
"numpydoc",
"pynndescent==0.5.8",
"pynndescent",
"pytest-benchmark",
"pytest-cases",
"pytest-cov",
Expand Down
Loading