switch from bundled lazy loading code to the public lazy_loader package #575

grlee77 · 2023-06-19T12:32:08Z

This PR switch from using the internally bundled _shared/lazy.py to the public lazy_loader package.

Also switches to use .pyi stub files in cases where submodules or attributes were being dynamically loaded.

closes #489 (probably, but have not yet verified from VS Code)

…lity

use .pyi type stubs

grlee77 · 2023-06-19T18:04:53Z

rebased on top of #574 which fixes some test failures with the most recent NumPy/CuPy

jakirkham · 2023-06-20T20:55:42Z

Thanks Greg! 🙏

Can you please update dependencies.yaml as well?

gigony

Thanks @grlee77 ! Looks good to me!

python/cucim/src/cucim/skimage/filters/_fft_based.py

…ader-and-type-stubs

jakirkham · 2023-07-25T23:00:11Z

Looks like there are some conflicts here that need to be resolved

…ader-and-type-stubs

jakirkham · 2023-07-26T19:57:35Z

Thanks Greg! 🙏

Could you please add lazy_loader >=0.1 to this package list?

cucim/dependencies.yaml

Lines 158 to 171 in 0a6bf42

    
           run: 
        
             common: 
        
               - output_types: conda 
        
                 packages: 
        
                   - click 
        
                   - cupy >=12.0.0 
        
                   - jbig 
        
                   - libwebp-base 
        
                   - numpy >=1.21.3 
        
                   - scikit-image >=0.19.0,<0.22.0a0 
        
                   - scipy 
        
                   - xz 
        
                   - zlib 
        
                   - zstd

jakirkham · 2023-07-26T20:18:40Z

The CI failure is due to the fact this change needs to be propagated

Please run (in some environment where this dependency can be installed):

python -m pip install rapids-dependency-file-generator
rapids-dependency-file-generator

Then commit & push the changes

jakirkham

Thanks Greg! 🙏

This is a nice simplification

Had a couple questions about files now included in distributions

python/cucim/MANIFEST.in

jakirkham · 2023-07-26T20:27:26Z

python/cucim/setup.py

@@ -39,6 +39,7 @@ def read(*names, **kwargs):
    url='https://developer.nvidia.com/multidimensional-image-processing',
    packages=find_packages('src'),
    package_dir={'cucim': 'src/cucim'},
+    package_data={"": ["*.pyi"]},  # distribute


Would it make sense to (somewhat?) align this with the MANIFEST.in?

not sure under what conditions it matters (e.g. python setup.py sdist seems to have the .npy files etc that are listed in the MANIFEST.in without listing them again here). I went ahead and added it for consistency, though.

Yeah the rule of thumb I try to use is...

If we want to ship them, but not install them, use MANIFEST.in

If we want to ship and install them, use package_data

Think the suggestion in comment ( #575 (comment) ) is revises this to the list we want to install. Though please let me know if we are still missing things

add .cu to MANIFEST.in

python/cucim/setup.py

Co-authored-by: jakirkham <[email protected]>

python/cucim/MANIFEST.in

Co-authored-by: jakirkham <[email protected]>

jakirkham

Thanks Greg! 🙏

jakirkham · 2023-07-26T21:17:09Z

(asking offline for OPS to review)

jakirkham · 2023-07-26T21:35:11Z

/merge

Fixes #6956 . ### Description Looks like an error which introduced by this [PR](rapidsai/cucim#575). The submodule in cucim.skimage can import correctly, so change to directly import cucim.skimage. ### Types of changes  - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: KumoLiu <[email protected]>

grlee77 added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 19, 2023

grlee77 added this to the v23.08.00 milestone Jun 19, 2023

grlee77 requested review from a team as code owners June 19, 2023 12:32

grlee77 added 3 commits June 19, 2023 13:51

enforce scalar low_threshold in _nonmaximum_suppression kernel for canny

8fe647c

make sure fft_shape is tuple and not ndarray for NumPy 1.25 compatibi…

6616ba1

…lity

switch from local _shared/lazy.py to lazy_loader package

9b3362c

use .pyi type stubs

grlee77 force-pushed the use-lazy_loader-and-type-stubs branch from 42a7c3b to 9b3362c Compare June 19, 2023 18:04

grlee77 added 2 commits June 19, 2023 14:31

add lazy_loader to conda environment files and cucim recipe

f2ebd76

revert change

9f2e5b9

gigony approved these changes Jul 11, 2023

View reviewed changes

python/cucim/src/cucim/skimage/filters/_fft_based.py Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/branch-23.08' into use-lazy_lo…

9252637

…ader-and-type-stubs

Merge remote-tracking branch 'upstream/branch-23.08' into use-lazy_lo…

3d7e0c3

…ader-and-type-stubs

add lazy_loader to dependencies.yaml

409d390

commit changes made by rapids-dependency-file-generator

c2a36ff

jakirkham reviewed Jul 26, 2023

View reviewed changes

grlee77 added 3 commits July 26, 2023 16:41

remove unused Cython and .ini types from MANIFEST.in

00a41e2

add .cu to MANIFEST.in

add .h and .cu for kernels to package_data

9f45567

add additional types to package_data field in setup.py

a99c1b0

jakirkham reviewed Jul 26, 2023

View reviewed changes

python/cucim/setup.py Outdated Show resolved Hide resolved

Update python/cucim/setup.py

dc29478

Co-authored-by: jakirkham <[email protected]>

jakirkham reviewed Jul 26, 2023

View reviewed changes

python/cucim/MANIFEST.in Outdated Show resolved Hide resolved

Update python/cucim/MANIFEST.in

85b47f6

Co-authored-by: jakirkham <[email protected]>

jakirkham approved these changes Jul 26, 2023

View reviewed changes

raydouglass approved these changes Jul 26, 2023

View reviewed changes

Merge branch 'branch-23.08' into use-lazy_loader-and-type-stubs

0224150

rapids-bot bot merged commit f8e9b38 into rapidsai:branch-23.08 Jul 26, 2023
21 checks passed

KumoLiu mentioned this pull request Sep 7, 2023

optional import cucim.skimage directly Project-MONAI/MONAI#6957

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch from bundled lazy loading code to the public lazy_loader package #575

switch from bundled lazy loading code to the public lazy_loader package #575

grlee77 commented Jun 19, 2023

grlee77 commented Jun 19, 2023

jakirkham commented Jun 20, 2023

gigony left a comment

jakirkham commented Jul 25, 2023

jakirkham commented Jul 26, 2023

jakirkham commented Jul 26, 2023

jakirkham left a comment

jakirkham Jul 26, 2023

grlee77 Jul 26, 2023

jakirkham Jul 26, 2023

jakirkham Jul 26, 2023

jakirkham left a comment

jakirkham commented Jul 26, 2023

jakirkham commented Jul 26, 2023

switch from bundled lazy loading code to the public lazy_loader package #575

switch from bundled lazy loading code to the public lazy_loader package #575

Conversation

grlee77 commented Jun 19, 2023

grlee77 commented Jun 19, 2023

jakirkham commented Jun 20, 2023

gigony left a comment

Choose a reason for hiding this comment

jakirkham commented Jul 25, 2023

jakirkham commented Jul 26, 2023

jakirkham commented Jul 26, 2023

jakirkham left a comment

Choose a reason for hiding this comment

jakirkham Jul 26, 2023

Choose a reason for hiding this comment

grlee77 Jul 26, 2023

Choose a reason for hiding this comment

jakirkham Jul 26, 2023

Choose a reason for hiding this comment

jakirkham Jul 26, 2023

Choose a reason for hiding this comment

jakirkham left a comment

Choose a reason for hiding this comment

jakirkham commented Jul 26, 2023

jakirkham commented Jul 26, 2023