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

Use string literal types in to_rgba() for matplotlib <3.8 #751

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

khaeru
Copy link
Contributor

@khaeru khaeru commented Feb 21, 2024

Hi—thanks for the fresh 0.13.0 release and continued improvement of the package 🙏🏾

Today we saw errors like this in GitHub Actions workflows that rely on plotnine, at this statement:

from matplotlib.typing import ColorType

These boil down to the fact that (a) matplotlib.typing does not exist in matplotlib <3.8 but (b) plotnine is compatible with matplotlib >=3.6:

"matplotlib>=3.6.0",

In short:

$ pip list | grep -E "plotnine|matplotlib"
matplotlib                             3.7.0
matplotlib-inline                      0.1.6
plotnine                               0.13.0.post1+gf5c01deb            /home/khaeru/vc/u/has2k1/plotnine

$ python -c "from plotnine._utils import to_rgba; to_rgba(None, None)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/khaeru/vc/u/has2k1/plotnine/plotnine/_utils/__init__.py", line 576, in to_rgba
    from matplotlib.typing import ColorType
ModuleNotFoundError: No module named 'matplotlib.typing'

This one commit fixes by removing the import and using string literal types within the function. The type checker should then use ColorType as imported within the if TYPE_CHECKING: block at the top of the file.

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f5c01de) 86.97% compared to head (47d2dc2) 86.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   86.97%   86.97%   -0.01%     
==========================================
  Files         179      179              
  Lines       11559    11558       -1     
  Branches     2116     2116              
==========================================
- Hits        10053    10052       -1     
  Misses       1017     1017              
  Partials      489      489              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@khaeru
Copy link
Contributor Author

khaeru commented Feb 21, 2024

Also, FWIW, in some GitHub Actions workflows I maintain I test against both the earliest and latest versions of key dependencies, for instance here. (Some projects use tox for the same purpose.)

I am not familiar with your CI strategy here so I did not hazard to make similar changes, but it can help catch such issues early. Please let me know if you'd like me to try for this PR.

@has2k1
Copy link
Owner

has2k1 commented Feb 21, 2024

Nice. Thanks for this

This one commit fixes by removing the import and using string literal types within the function. The type checker should then use ColorType as imported within the if TYPE_CHECKING: block at the top of the file.

I never thought of using string literals in cast to avoid runtime imports!

@has2k1
Copy link
Owner

has2k1 commented Feb 21, 2024

Also, FWIW, in some GitHub Actions workflows I maintain I test against both the earliest and latest versions of key dependencies, for instance here. (Some projects use tox for the same purpose.)

I considered testing against the against the lowest supported package versions but I felt that I wouldn't keep up with updating the dependencies in two places. The place for these tests is in the weekly tests, which aims to keep track of dependencies.

However Astral's new package installer uv allows installing minimum versions for the dependencies and we can now use that.

@has2k1 has2k1 merged commit 6f6a770 into has2k1:main Feb 21, 2024
8 checks passed
khaeru added a commit to khaeru/genno that referenced this pull request Feb 26, 2024
Install a different version in "pytest" CI workflow
khaeru added a commit to khaeru/genno that referenced this pull request Feb 26, 2024
Install a different version in "pytest" CI workflow
khaeru added a commit to khaeru/genno that referenced this pull request Mar 14, 2024
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Jul 29, 2024
pyam pins matplotlib <3.7.1, which causes message_data to run into
has2k1/plotnine#751. override.
khaeru added a commit to iiasa/message-ix-models that referenced this pull request Jul 30, 2024
pyam pins matplotlib <3.7.1, which causes message_data to run into
has2k1/plotnine#751. override.
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