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 inspect to determine whether the AnyIO version supports abandon_on_cancel #224

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Stealthii
Copy link

@Stealthii Stealthii commented Sep 19, 2024

#202 introduces an issue on py38 and py39 with an earlier version of importlib.metadata not detecting versions in certain virtual environments. It also uses string comparisons:

Traceback (most recent call last):
  File "example/lib64/python3.8/site-packages/asyncer/__init__.py", line 3, in <module>
    from ._main import PendingValueException as PendingValueException
  File "example/lib64/python3.8/site-packages/asyncer/_main.py", line 17, in <module>
    from asyncer._compat import run_sync
  File "example/lib64/python3.8/site-packages/asyncer/_compat.py", line 16, in <module>
    if ANYIO_VERSION >= "4.1.0":
TypeError: '>=' not supported between instances of 'NoneType' and 'str'

This PR addresses the issue by using inspect to determine anyio's supported kwargs for the run_sync method.

@Stealthii
Copy link
Author

Stealthii commented Sep 19, 2024

This bug affects v0.0.8 (the latest version) only.

The PR also contains as prior solution to correctly detect AnyIO package version using importlib-metadata and numeric comparison for Python < 3.10, however this was ultimately replaced with the kwargs check.

We should avoid relying on importlib.metadata outside of build environments as it relies heavily on discoverable dist-info or egg-info metadata, which may not be available in non-virtualenv setups (such as system-level packages).

@Stealthii
Copy link
Author

@tiangolo I would recommend merging and releasing this in the next version before dropping Python 3.8 support.

@Stealthii
Copy link
Author

This still affects non-venv environments. This should be merged and released, as any packages using this that are installed as part of a system install (or flat package) have to explicitly block v0.0.8 from dependency mapping.

@Stealthii Stealthii force-pushed the bugfix/importlib-metadata-py38 branch from ffa8ec8 to 7c734f1 Compare January 30, 2025 19:57
Copy link

@auvipy auvipy 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 check why the check level is failing?

@svlandeg svlandeg added the bug Something isn't working label Feb 24, 2025
@svlandeg svlandeg changed the title Correctly determine AnyIO support kwargs for run_sync 🐛 Use inspect to determine whether the AnyIO version supports abandon_on_cancel Feb 24, 2025
@svlandeg svlandeg self-assigned this Feb 24, 2025
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Hi @Stealthii, thanks for the contribution, and apologies for the delay in reviewing this!

Comparing the ANYIO_VERSION with a string comparison is definitely a bug, and something we'll make sure to avoid in the future.

I think it makes sense to use a direct inspect call to check the availability of the correct argument. All the changes in this PR look good to me, and it even removes some duplicate code which is great 💯

@svlandeg svlandeg removed their assignment Feb 26, 2025
Copy link
Member

@tiangolo tiangolo 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! 🚀

There's one small issue with the current implementation, the check for abandon_on_cancel will be executed every time run_sync is called. With something closer to the previous implementation (with some duplicated code) the check is run only once when declaring the function, but after that, one of the two versions of the function will be the only one declared and executed.

Maybe we could move the if statement to the position where the previous (and problematic) check was.

@svlandeg svlandeg marked this pull request as draft February 28, 2025 14:42
@Stealthii Stealthii force-pushed the bugfix/importlib-metadata-py38 branch from a947837 to 4223411 Compare March 1, 2025 02:07
importlib.metadata in Python 3.10 had improved handling of certain
virtual environments. This change addresses the issue with some venv
environments not returning a version string for AnyIO.
This should keep type checkers happy that aren't running code.
This ensures the check is only performed once on import, whilst
retaining a static function definition.
@Stealthii Stealthii force-pushed the bugfix/importlib-metadata-py38 branch from 4223411 to 0227077 Compare March 1, 2025 02:08
@Stealthii
Copy link
Author

Agreed @tiangolo - oversight on my part when throwing that into the ring when rebasing recently!

I can revert 78626db, or perhaps move the kwarg check to a constant on import? (done in 0227077). Don't mind either way, just conscious I might not be matching codestyle on this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants