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

Change how cupy import fall-through is handled #533

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

sezelt
Copy link
Member

@sezelt sezelt commented Oct 9, 2023

Currently, we have many instances of this pattern:

try:
    import cupy as cp
except:
    cp = None

This is done to ensure that we can write code that mixes usage of numpy and cupy while still importing in an environment that doesn't have cupy installed. Selection between the modules is done through logic within the code (and this is always necessary because even when cupy is present it's not optimal to always replace numpy with cupy).

This pattern causes two problems:

  1. It suppresses all errors raised by import cupy as cp. On many systems, cupy is present but misconfigured, and the errors it emits to warn users of this are caught and dropped by this code.
  2. Setting cp = None makes the eventual error that appears in the former scenario extremely confusing (usually something along the lines of AttributeError: 'NoneType' object has no attribute 'asnumpy'. This also interferes with static analysis, and causes it to flag any use of cp since it (correctly) infers that cp can equal either cupy or None.

This PR changes these two behaviors by replacing the above pattern with the following:

try:
    import cupy as cp
except ModuleNotFoundError:
    cp = np

Issue (1) is addressed by only capturing ModuleNotFoundError, so that if cupy is present but misconfigured, its import error gets properly raised, and issue (2) is... different. In the case where cupy was misconfigured, we will fail much earlier and in a clearer way, so the user never reaches the call to None.asnumpy. In the case of the static analyzer, in an environment without cupy it will flag only methods that exist in cupy but not numpy, reducing but not eliminating the false positives. The only oddly behaved case is if a user uses device='gpu' in an environment that don't have cupy installed at all, in which case they will eventually encounter some AttributeError when a cupy-only function is called on numpy.

Merging closes #527.

Copy link
Collaborator

@alex-rakowski alex-rakowski left a comment

Choose a reason for hiding this comment

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

looks good

@bsavitzky
Copy link
Member

Looks good here too. When we set up persistent config support such that device='gpu'-like patterns are handled once/in the backend let's add a check for a cupy installation there, which should take care of the remaining issue you've noted, @sezelt. Otherwise if we run into any other weird AttributeError: module 'numpy' has no attribute 'crazythings' we'll know where to look....

@bsavitzky bsavitzky merged commit e8305e7 into py4dstem:dev Oct 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cupy import errors are suppressed
3 participants