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.

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

Fix ImportError from vendored code #252

Merged

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Mar 31, 2022

closes #251 by avoiding imports from private CuPy code

The new files here are just subsets of files present in CuPy that are not part of its public API. Only a few functions use functionality from this _vendored folder.

The whole folder is mainly here in order to have a version of cupyx.scipy.signal.fftconvolve with a modified choose_conv_mode. We should eventually just update the convolution selection code in CuPy so that we can remove these files.

I think fftconvolve itself was not present at all in CuPy 9.x, but even in the current development branch, the convolution choice function is still restricted to 1D

@grlee77 grlee77 added bug Something isn't working non-breaking Introduces a non-breaking change labels Mar 31, 2022
@grlee77 grlee77 requested a review from a team as a code owner March 31, 2022 01:54
@jakirkham
Copy link
Member

Thanks Greg! 😄

Instead of vendoring what if we do a try...except... to handle importing with the relocation?

Agree in the medium term would be good to contribute upstream to avoid needing these internal bits.

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 31, 2022

what if we do a try...except... to handle importing with the relocation?

Do you mean modifying _ndimage_filters.py and others to something like this?

try:
    import cupyx.scipy.ndimage

    if hasattr(cupyx.scipy.ndimage, '_filters'):
        # CuPy 11.x
        from cupyx.scipy.ndimage._filters import _get_correlate_kernel
    else:
        from cupyx.scipy.ndimage.filters import _get_correlate_kernel

except ImportError:

    import cupy

    from cucim.skimage._vendored import _ndimage_filters_core as _filters_core


    @cupy.memoize(for_each_device=True)
    def _get_correlate_kernel(mode, w_shape, int_type, offsets, cval):
        return _filters_core._generate_nd_kernel(
            'correlate',
            'W sum = (W)0;',
            'sum += cast<W>({value}) * wval;',
            'y = cast<Y>(sum);',
            mode, w_shape, int_type, offsets, cval, ctype='W')

That would let us potentially benefit from any upstream improvements or fixes to _get_correlate_kernel, but I don't think that outweighs the risk that the behavior may change in unexpected ways?

@jakirkham
Copy link
Member

Alright let's go with this as-is, but let's work on upstreaming the rest of what is needed for 22.06. Could you please file an issue to track this?

@jakirkham
Copy link
Member

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ImportError with development branch of CuPy
2 participants