Skip to content

Allow imports to work if openequivariance is installed but not running on supported GPU - #1534

Open
bernstei wants to merge 1 commit into
developfrom
oeq_cpu_workaround
Open

Allow imports to work if openequivariance is installed but not running on supported GPU#1534
bernstei wants to merge 1 commit into
developfrom
oeq_cpu_workaround

Conversation

@bernstei

Copy link
Copy Markdown
Collaborator

Check for AssertionError that contains a specific string when importing openequivariance, and allow it to continue without OEQ if it's caused by running on a non-supported GPU (e.g. torch CPU)

closes #1532

@aacostadiaz aacostadiaz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! One small thing below, otherwise looks good to me.

OEQ_AVAILABLE = True
except ImportError:
OEQ_AVAILABLE = False
except AssertionError as exc:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two test files still have the same crash: tests/helpers.py and tests/backends/backend_parity.py. In those two files, catching AssertionError too is enough.

except (ImportError, AssertionError):
    OEQ_AVAILABLE = False

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure that catching just any generic assertion error is enough, but I was trying to be careful, so that only the particular backend-related assertion was caught, to allow for other failure modes to still lead to an exception.

Do you not think we should apply that level of specificity to the tests as well? Who knows what other assertions those imports could trigger.

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.

2 participants