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

Fix [BUG] Module 'PIL' has no attribute Image #1793

Closed

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Aug 13, 2023

TL;DR

Please replace this text with a description of what this PR accomplishes.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

You can search "return PIL.Image.open(local_path)" in the code, and you will find that it is not enough to just use "import PIL" in the code.

Tracking Issue

flyteorg/flyte#3813

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@@ -1,5 +1,7 @@
from typing import TYPE_CHECKING, List, Optional, Union

import PIL.Image
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we need this? We just use PIL.Image.Image so line 17 should be enough right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are probably right.

Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

I do not think we should add the import, there is a lazy load already

@Future-Outlier
Copy link
Member Author

I do not think we should add the import, there is a lazy load already

Just found that you are right, thanks a lot for the review!

@Future-Outlier
Copy link
Member Author

Future-Outlier commented Aug 15, 2023

Just contact the author of this pull request, I think maybe the bug doesn't exist.
https://github.com/flyteorg/flytekit/blob/master/plugins/flytekit-deck-standard/tests/test_renderer.py#L60-L72
Our test has covered the case he mentioned.

@Future-Outlier Future-Outlier marked this pull request as draft August 16, 2023 02:48
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