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

add support for async contextvars #153

Closed
wants to merge 7 commits into from
Closed

Conversation

n1ywb
Copy link

@n1ywb n1ywb commented Apr 18, 2020

this ugly hack causes the context to be preserved across all excursions into async land. this fixes #127

@n1ywb
Copy link
Author

n1ywb commented Apr 18, 2020

works on python 3.8 at least 😅

@simonfagerholm
Copy link
Contributor

Why are the hypothesis tests removed?

@n1ywb
Copy link
Author

n1ywb commented Apr 18, 2020

Why are the hypothesis tests removed?

oops

@n1ywb
Copy link
Author

n1ywb commented Apr 18, 2020

Why are the hypothesis tests removed?

honestly its because my internet went down last night and I couldn't install the hypothesis package at the time

@n1ywb
Copy link
Author

n1ywb commented Apr 18, 2020

one caveat I might mention about this implementation is that EVERY async task will run in the same context. normally child tasks would inherit a COPY of the parent task's context. but it's probably an acceptable trade off in a test environment. but it could cause context to unexpectedly leak from a child task into a parent task.

it's fixable by making the root task a special case that gets the original context while child tasks get copies. but not necessary for my use case ;)

@n1ywb
Copy link
Author

n1ywb commented Apr 18, 2020

I opened https://bugs.python.org/issue40320 to officially request the ability to pass context to task

@achimnol
Copy link

I just hit a similar problem regarding contextvars with async-yield fixtures.
Could your PR also resolve this case as well?

achimnol added a commit to lablup/backend.ai-client-py that referenced this pull request Apr 19, 2020
* All test cases in tests/test_cli_proxy.py are marked "xfail"
  because there is an upstream issue rendering those tests always
  failing while real-world use cases have no problems.

  - ref) pytest-dev/pytest-asyncio#153
achimnol added a commit to lablup/backend.ai-client-py that referenced this pull request Apr 19, 2020
* feat: Add background task support
* feat: Implement progress bar for rescanning images using bgtask API
* refactor: Clean up BaseFunction inheritance using contextvars
  - Previously, to bind the current session with API function classes,
    we generated new type objects at runtime.
    - This has confused IDEs and type checkers.
    - Now type checkers can statically deduce the types for individual API
      function classes.
  - Now we use contextvars (ai.backend.client.session.api_session)
    to keep the reference to the current session.
    - There are *NO* public Session/AsyncSession API changes!
    - Only the API function classes need to be rewritten.
    - For synchronous Session, we pass the context to the separate worker
    thread using copy_context() whenever calling API functions, which is
    a light-weight operation.
* fix: Remove redundant src/ai/backend/client/etcd.py which had been already
  copied to src/ai/backend/client/func/etcd.py
* ci: Bump the lint Python version to 3.7
* ci: Remove Python 3.6 from AppVeyor
* ci: Make Python 3.8 as the officially supported environment by removing it
  from "allow_failures" environments in Travis CI
* ci: Now type errors are no longer ignored, and all remaining type errors are
  now fixed.
* test: All test cases in tests/cli/test_cli_proxy.py are marked "xfail"
  because there is an upstream issue rendering those tests always
  failing while real-world use cases have no problems.
  - ref) pytest-dev/pytest-asyncio#153

BREAKING-CHANGE: Dropped Python 3.6 support. Now it requires Python 3.7 or higher.
@n1ywb
Copy link
Author

n1ywb commented Apr 20, 2020

I just hit a similar problem regarding contextvars with async-yield fixtures.
Could your PR also resolve this case as well?

It should. I use them in my app tests which is the reason I did this.

@jpic
Copy link

jpic commented Jan 10, 2021

Works great! Contextvars are such a neat feature ...

Just had to do a minor rebase the branche because master changed from doing "append if not in item.fixturenames" with "remove if in item.fixturenames then insert".

Thank you very much! 🎩

@lephuongbg
Copy link

Verified that this one works. I like this one more than #161 since I can easily copy the necessary parts to my conftest.py.

@mr-rodgers
Copy link

Is this fix abandoned?

@seifertm
Copy link
Contributor

seifertm commented Sep 6, 2022

I really hate to do this, but I'm closing this PR in favour of the solution proposed in #312. I'll my best to outline my reasons.

I like that you found a nice Task abstraction. It has a high "code locality" (i.e. the functionality coherent and found mostly inside Task). The PR also provides the appropriates tests.

However, the Task class makes use of protected types, such as asyncio.tasks._PyTask and asyncio.futures._PyFuture. Their names or functionality might change between patch releases. As we only test with different minor versions of Python, we likely wouldn't be able to discover any breakage.

The PR also introduces the context fixture, which I see as problematic. Pytest-asyncio currently encourages overriding the event_loop fixture, in order to control the fixture scope. Unfortunately, people tend to write all sorts of code into their event_loop fixtures, in order to "inject" functionality into their tests. This makes it hard to maintain pytest-asyncio as the custom code in the loop fixture causes additional corner cases that break when changing the plugin code. Therefore, we should try to reduce the number of fixtures provided by pytest-asyncio, rather than adding more (see #311). I fear that adding the context fixture will open another can of worms similar to event_loop.

Python 3.11 introduces asyncio.Runner, which essentially allows running multiple async functions in the same context. #312 introduces the concept of a Runner in pytest-asyncio. I think this is the approach we should take

Others have reported they could use similar code in their conftest.py, so this may serve them as a workaround, until #127 is fixed.

I'm sorry this PR was stale for such a long time. A single maintainer can only spend so much time and we only "added staff" in the beginning of this year.

@seifertm seifertm closed this Sep 6, 2022
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.

ContextVar not propagated from fixture to test
7 participants