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

add kiwisolver not install warning for import error #1019

Merged
merged 5 commits into from
Apr 4, 2023

Conversation

homosapien-lcy
Copy link
Contributor

@homosapien-lcy homosapien-lcy commented Mar 30, 2023

Now, kiwisolver is listed as optional but in some demo as mentioned in issue #1018, it can raise import error. This PR gives a specific error when it's not installed and the related tests are run. Closes #1018

@dpinte
Copy link
Member

dpinte commented Mar 30, 2023

As discussed during the standup, let's move from using print to warnings.warn

@mdickinson
Copy link
Member

Just to make sure I understand: the problem being solved here is that while kiwisolver is optional for Enable itself, some of the Enable examples use it. Is that correct?

If so, I don't think we want any sort of warning for general Enable use (e.g., when importing enable.api). The warning should be limited to the examples that need it.

@homosapien-lcy
Copy link
Contributor Author

Just to make sure I understand: the problem being solved here is that while kiwisolver is optional for Enable itself, some of the Enable examples use it. Is that correct?

If so, I don't think we want any sort of warning for general Enable use (e.g., when importing enable.api). The warning should be limited to the examples that need it.

Thanks for the comments. There are about 10 tests that are using this package, it seems inefficient if we try to add individual warnings to each of them. Is there a way we can add warning for all the imports (like wrapping imports with a decorator)?

@corranwebster
Copy link
Contributor

corranwebster commented Mar 30, 2023

There are about 10 tests that are using this package.

For tests that require kiwisolver you want to skip the tests unless it is installed, not warn. But also it should possibly be in the "test" optional dependencies.

Edit to add: which it looks is exactly what the tests are doing.

@mdickinson
Copy link
Member

There are about 10 tests that are using this package

I think the tests are fine as they are - we're already skipping the tests for ConstraintsContainer if kiwisolver is not installed. I don't see any need to change anything there.

For the examples, I see two examples that use the ConstraintsContainer: contraints_demo and image_draw. The goal here would be to get a better error message for someone who runs one of those examples without having kiwisolver installed.

One other possibility, available since Python 3.7 (see PEP 562), would be to add a module-level __getattr__ to the enable.api module so that when someone tries to explicitly use ConstraintsContainer from enable.api, they get a clear message that kiwisolver is required. (The key point here is that there's currently no warning for the majority of Enable users who don't have kiwisolver installed and don't care about the ConstraintsContainer. We don't want to introduce such a warning, since such a warning would be unnecessary noise.)

@dpinte
Copy link
Member

dpinte commented Mar 31, 2023

Thanks @mdickinson and @corranwebster. Agreed on not needing any noise with the warnings for general enable use.

@homosapien-lcy as @mdickinson suggests, let's just get the two examples updated with a better error message.

@homosapien-lcy
Copy link
Contributor Author

Thanks @mdickinson and @corranwebster. Agreed on not needing any noise with the warnings for general enable use.

@homosapien-lcy as @mdickinson suggests, let's just get the two examples updated with a better error message.

OK, got it fixed

enable/api.py Outdated Show resolved Hide resolved
enable/coordinate_box.py Outdated Show resolved Hide resolved
@mdickinson mdickinson mentioned this pull request Apr 3, 2023
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM

@homosapien-lcy homosapien-lcy merged commit daf6fcc into main Apr 4, 2023
@homosapien-lcy homosapien-lcy deleted the kivisolver_check branch April 4, 2023 00:19
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.

ImportError: cannot import name 'ConstraintsContainer' from 'enable.api' when running python3.11
4 participants