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

Update typing_extension Any import to typing due to typing_extension version compatibility #1877

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

hfurkanvural
Copy link
Contributor

TL;DR

import of type Any in flytekit/core/array_node_map_task.py:9 is not supported for the typing_extensions libraries previous versions. This version range should fix it, otherwise dependency resolution might cause the following error:

import flytekit
/usr/local/lib/python3.8/dist-packages/flytekit/__init__.py:240: in <module>
    from flytekit.sensor.sensor_engine import SensorEngine
/usr/local/lib/python3.8/dist-packages/flytekit/sensor/__init__.py:1: in <module>
    from .base_sensor import BaseSensor
/usr/local/lib/python3.8/dist-packages/flytekit/sensor/base_sensor.py:12: in <module>
    from flytekit.extend.backend.base_agent import AsyncAgentExecutorMixin
/usr/local/lib/python3.8/dist-packages/flytekit/extend/__init__.py:47: in <module>
    from flytekit.tools.translator import get_serializable
/usr/local/lib/python3.8/dist-packages/flytekit/tools/translator.py:10: in <module>
    from flytekit.core.array_node_map_task import ArrayNodeMapTask
/usr/local/lib/python3.8/dist-packages/flytekit/core/array_node_map_task.py:9: in <module>
    from typing_extensions import Any
E   ImportError: cannot import name 'Any' from 'typing_extensions' (/usr/local/lib/python3.8/dist-packages/typing_extensions.py)

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

Due to too broad version range of typing_extensions extensions, dependency resolution does not get the right version sometimes. And code base has a specific type which is introduced on version 4.4.0. So this will fix/prevent the import issue.

Tracking Issue

NA

Follow-up issue

NA

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Could you update that line instead?
it should be

from typing import Any

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3866bf9) 55.05% compared to head (5b5f1d0) 54.85%.
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1877      +/-   ##
==========================================
- Coverage   55.05%   54.85%   -0.21%     
==========================================
  Files         296      293       -3     
  Lines       22241    22144      -97     
  Branches     3356     3357       +1     
==========================================
- Hits        12244    12146      -98     
- Misses       9834     9835       +1     
  Partials      163      163              
Files Coverage Δ
flytekit/core/array_node_map_task.py 36.14% <100.00%> (-0.39%) ⬇️

... and 7 files with indirect coverage changes

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

@hfurkanvural
Copy link
Contributor Author

Could you update that line instead? it should be

from typing import Any

Reverted my change on version and changed that line. Thanks for the help :)
@pingsutw

@hfurkanvural hfurkanvural changed the title Update typing_extensions version range to >=4.4.0 Update typing_extension Any import to typing due to typing_extension version compatibility Oct 9, 2023
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

pingsutw
pingsutw previously approved these changes Oct 9, 2023
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

There is a lint error, could you run make fmt locally, and push again

[INFO] This may take a few minutes...
flake8...................................................................Passed
black....................................................................Passed
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /home/runner/work/flytekit/flytekit/flytekit/core/array_node_map_task.py

check yaml...............................................................Passed

Signed-off-by: H. Furkan Vural <[email protected]>
@hfurkanvural
Copy link
Contributor Author

Updated, thanks @pingsutw :)

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Thank you!

@eapolinario
Copy link
Collaborator

@hfurkanvural , can you confirm which versions of python you tested and typing_extensions you tested?

python/typing_extensions@7d1aeea is the commit where it was introduced, which indicates that it should have been enabled for all versions of typing_extensions >= 4.4.0.

I'm not sure which version on python 3.8 introduced Any though.

@pingsutw
Copy link
Member

I use Python 3.8.8, and run into the same issue

@hfurkanvural
Copy link
Contributor Author

@eapolinario I use python 3.8.10

@hfurkanvural
Copy link
Contributor Author

@pingsutw since codecov is reduced with my latest change, i can not merge the PR. do you have any suggestion to fix that?
As I didnt really change anything in the code level, I can t tell why it s lower now

@pingsutw pingsutw merged commit 469d7f1 into flyteorg:master Oct 11, 2023
67 of 68 checks passed
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