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 sys.version_info to guard imports #1177

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

alicederyn
Copy link
Collaborator

Pull Request Checklist

Description of PR
Currently, conditional imports that depend on Python version are using try/except ImportError to import from typing_extensions on older Python versions. This is not natively supported by typecheckers, forcing use of error suppression, and losing potential validation of downstream code. (Additionally, pyright is erroneously flagging a lot of code that uses Annotated with "Call expression not allowed in type expression, making it harder to spot real issues in VSCode.)

This PR refactors these conditional imports to use the recommended if sys.version_info >= (3, minor): pattern instead.

See also: mypy issue comment, pyright issue comment

Comment on lines +11 to +17
if sys.version_info >= (3, 10):
from typing import get_args, get_origin
else:
# Python 3.8 has get_origin() and get_args() but those implementations aren't
# Annotated-aware. Python 3.9's versions don't support ParamSpecArgs and
# ParamSpecKwargs.
from typing_extensions import get_args, get_origin
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old version of this code was importing Self in the try/except block, causing it to fall back to the bespoke implementation in typing_extensions for Python 3.9. I'm unclear if this was intentional, so retained this behaviour to avoid any potential regression. For Python 3.10, typing_extensions imports Python's built-in version, so we can import it directly.

else:
from typing_extensions import Annotated


from typing_extensions import ParamSpec, get_args, get_origin
Copy link
Collaborator Author

@alicederyn alicederyn Aug 27, 2024

Choose a reason for hiding this comment

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

Unclear if this was intentionally not guarded, so left as-is. If the intention is to ensure any future typing_extensions behavioural changes are included, we could in a future PR change this to

if TYPE_CHECKING:
  if sys.version_info >= (3, 9):
    from typing import ParamSpec, get_args, get_origin
  else:
    from typing_extensions import ParamSpec, get_args, get_origin
else:
  try:
    # Prefer to use get_args/get_origin from typing_extensions to support any future behavioural
    # extensions
    from typing_extensions import ParamSpec, get_args, get_origin
  except ImportError:
    # Fall back to typing if typing_extensions is not available
    from typing import ParamSpec, get_args, get_origin

@alicederyn alicederyn force-pushed the conditional.imports branch from 2dc6f22 to e0bcd68 Compare August 27, 2024 14:02
Use `if sys.version_info >= (3, minor):` for Python-version-dependent imports. Unlike `try/except ImportError`, this is natively supported by typecheckers, allowing removal of typechecker error suppression and consequently better typechecking validation.

Signed-off-by: Alice Purcell <[email protected]>
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.5%. Comparing base (d553546) to head (90a7a2e).
Report is 190 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1177     +/-   ##
=======================================
- Coverage   81.7%   81.5%   -0.2%     
=======================================
  Files         54      60      +6     
  Lines       4208    4756    +548     
  Branches     889    1016    +127     
=======================================
+ Hits        3439    3880    +441     
- Misses       574     646     +72     
- Partials     195     230     +35     

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

@alicederyn alicederyn force-pushed the conditional.imports branch from e0bcd68 to 91294df Compare August 27, 2024 14:04
@alicederyn alicederyn added semver:patch A change requiring a patch version bump type:enhancement A general enhancement python Dependabot pull requests that update Python dependencies labels Aug 27, 2024
@alicederyn alicederyn marked this pull request as ready for review August 27, 2024 14:32
Copy link
Collaborator

@jeongukjae jeongukjae left a comment

Choose a reason for hiding this comment

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

👍

else:
from typing_extensions import Annotated


from typing_extensions import ParamSpec, get_args, get_origin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not strong opinion. looks great if we update this statement as well.

Copy link
Collaborator Author

@alicederyn alicederyn Aug 28, 2024

Choose a reason for hiding this comment

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

👍 Will leave to a separate PR to avoid risking coupling a behaviour change to a large refactor

@sambhav sambhav merged commit 9e9a772 into argoproj-labs:main Aug 28, 2024
19 of 20 checks passed
@alicederyn alicederyn deleted the conditional.imports branch August 28, 2024 11:48
@elliotgunton elliotgunton removed the python Dependabot pull requests that update Python dependencies label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants