Skip to content

Fix capitol image#880

Merged
dpinte merged 9 commits into
mainfrom
fix_capitol_image
May 8, 2023
Merged

Fix capitol image#880
dpinte merged 9 commits into
mainfrom
fix_capitol_image

Conversation

@homosapien-lcy
Copy link
Copy Markdown
Contributor

After the capitol.jpg moved, chaco/examples/user_guide/plot_types/create_plot_snapshots is broken, use importlib.resources to find the image for loading to fix this

@homosapien-lcy
Copy link
Copy Markdown
Contributor Author

closes #879

Copy link
Copy Markdown
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

This breaks on 3.7 and 3.8.

See https://github.com/enthought/pyface/blob/0fb8373b4dbb5e104d3120e88590dbf964865245/pyface/tests/test_pil_image.py#L14-L20 for an example of how to handle this sort of use of importlib across many Python versions

from importlib import resources

filename = os.path.join("..", "..", "demo", "basic", "capitol.jpg")
filename = resources.files(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

resources.files is only available on Python 3.9 or later, so this will break on 3.7 and 3.8 which we still want to support.

The fix is to use importlib_resources package which provides the newer API on older versions of Python

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, just add both importlibs

Copy link
Copy Markdown
Member

@dpinte dpinte left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 446 to 449
filename = files(
'chaco.examples.demo.basic'
).joinpath('capitol.jpg')
image_source = ImageData.fromfile(filename)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typically you want to have this in a with statement something like:

resource = files(...)
with as_file(resource) as filename:
    ...

Copy link
Copy Markdown
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Can you please add an appropriate dependency to importlib_resources for Python < 3.9 here: https://github.com/enthought/chaco/blob/main/chaco/__init__.py#L19

Also a usage issue for importlib.resources.files.

@homosapien-lcy
Copy link
Copy Markdown
Contributor Author

Can you please add an appropriate dependency to importlib_resources for Python < 3.9 here: https://github.com/enthought/chaco/blob/main/chaco/__init__.py#L19

Also a usage issue for importlib.resources.files.

Got it, just add a "check python3.9 then install importlib_resources" there.
Just want to clarify, what do you mean by usage issue for importlib.resources.files?

@corranwebster
Copy link
Copy Markdown
Contributor

Just want to clarify, what do you mean by usage issue for importlib.resources.files?

Meaning it should be used in a with statement, which you have now done.

This should be good to merge.

@homosapien-lcy
Copy link
Copy Markdown
Contributor Author

Just want to clarify, what do you mean by usage issue for importlib.resources.files?

Meaning it should be used in a with statement, which you have now done.

This should be good to merge.

Thanks Corran! The merge is still block by the ci test, the good old RuntimeError Numpy... Is there anyway we can get around it?

RuntimeError: NumPy was built with baseline optimizations: 
(SSE SSE2 SSE3 SSSE3 SSE41 POPCNT SSE42 AVX F16C AVX2) but your machine doesn't support:
(AVX2).

@dpinte dpinte merged commit 680a6e6 into main May 8, 2023
@dpinte dpinte deleted the fix_capitol_image branch May 8, 2023 08:21
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.

3 participants