Add contextvars support.

[Fuyukai]

This adds PEP 567 contextvars support to the core of Trio.

Todo:

• Tests
• Deprecate TaskLocal
• Wait for a backport for 3.5 and 3.6.

Closes #420 and closes #417, see also: #178.

[Fuyukai]

Note the tests will not pass for this at all because I import contextvars which obviously doesn't work on 3.5/3.6 yet.

[codecov]

Codecov Report

Merging #478 into master will decrease coverage by 0.07%.
The diff coverage is 94.19%.

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
- Coverage   99.26%   99.19%   -0.08%     
==========================================
  Files          89       89              
  Lines       10380    10509     +129     
  Branches      721      728       +7     
==========================================
+ Hits        10304    10424     +120     
- Misses         58       65       +7     
- Partials       18       20       +2

Impacted Files	Coverage Δ
trio/_toplevel_core_reexports.py	100% <ø> (ø)	⬆️
trio/_core/tests/test_local.py	100% <100%> (ø)	⬆️
trio/_core/tests/test_run.py	100% <100%> (ø)	⬆️
trio/_core/_run.py	100% <100%> (ø)	⬆️
trio/_core/_local.py	91% <85%> (-9%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aee2676...4a63ec6. Read the comment docs.

[njsmith]

I guess we'll also want to replace RunLocal with some sort of RunVar interface that's modeled after ContextVar.

@1st1 would you have any objection to is releasing a contextvars package on pypi, that (initially) just has a pure python implementation of the contextvars API?

[1st1]

Would you mind to wait a couple of days for this? I will try to find time to do this.

[1st1]

Here we go: https://pypi.python.org/pypi/pep567/0.1.0

[1st1]

And here's the repo: https://github.com/MagicStack/pep567 Please feel free to file issue/requests. I can make you a co-maintainer Nathaniel if you need that.

[njsmith]

Huh, I figured we could just call the module contextvars, so that everyone could depend on contextvars; python_version < "3.7" and then import contextvars and not worry about what version they're on. Is there some reason you didn't go that way? (Note that if there's a name conflict between the stdlib and site-packages, the stdlib wins.)

[1st1]

"contextvars" name is taken on PyPI.

[njsmith]

Heh, whoops. Though, the import name doesn't have to match the pypi name, so it could still be import contextvars. Or maybe that person could be convinced to give up their not-touched-in-the-last-6-years package given that the stdlib has claimed the name anyway...

[1st1]

Though, the import name doesn't have to match the pypi name, so it could still be import contextvars

Yeah, but that can be really confusing to some users (I'd be confused).

Or I bet that person could be convinced to give up their not-touched-in-the-last-6-years package given that the stdlib has claimed the name anyway...

Maybe, the problem is that someone might still be using that old package. In the end, I thought that the name of the module is not that important...

[njsmith]

@1st1 if it were just trio/asyncio/... who had to notice this I wouldn't care, but everyone who wants to use a ContextVar has to do the same import dance, so we have to include it in all our docs examples, etc. And if anyone is using that old package then, well, they're screwed when 3.7 comes out anyway :-). Also, it looks like it's only getting ~1 download/week. So... would you mind if I sent the owner a quick email to find out if they'd be amenable?

[1st1]

Sure, please do. I'd be totally OK to rename the package.

[njsmith]

Email sent!

[1st1]

Thanks! :)

[njsmith]

@Fuyukai If you want to keep making progress on this while we're waiting to hear back, I guess you can start with using pep567 – should be easy to switch if the name changes.

You'll want to add an entry like:

":python_version < '3.7'": ["pep567"]

to the extras_require dict in setup.py, and then it should start installing automatically on the older test runs.

We will want to deprecate TaskLocal; this requires:

• adding the warning (I think just decorating TaskLocal.__init__)

• Adding a removal newsfragment

• Deleting the TaskLocal docs

Also, do you want to handle the analogous RunLocal → RunVar transition in this PR too, or leave that for later?

(And thanks for working on this btw!)

[Fuyukai]

":python_version < '3.7'": ["pep567"]

TIL this is a thing.

But sure, I'll do those changes.

[1st1]

Alright, thanks to Nathaniel, the backport is now available under "contextvars" name :)

So please update the dependency in your PR to "contextvars".

[njsmith]

Sweet!

@1st1 it looks like now the PR tests here are failing because your backport package only supports 3.6, not 3.5. Is that intentional? I know f strings are hard to give up... :-)

[1st1]

@1st1 it looks like now the PR tests here are failing because your backport package only supports 3.6, not 3.5. Is that intentional? I know f strings are hard to give up... :-)

They are indeed hard to give up :) contextvars@2.1 is now 3.5 compatible.

Are there many trio users still using 3.5? I'm considering dropping its support from uvloop/asyncpg, but will probably wait until 3.7 comes out.

[Fuyukai]

Deleting the TaskLocal docs

I don't think this should be done; they should be changed to mention ContextVar objects instead, since the majority of the section deals with the issue more abstractly and just autoclasses TaskLocal after (Unless this is what you meant?)

[njsmith]

@Fuyukai

Yes, that's what I mean, sorry. The point is that we don't mention deprecated features in the docs, which might not be obvious. (Except for in the change log, of course.)

@1st1

Are there many trio users still using 3.5?

I'm not sure, but we do still support it for now. The main reasons are (1) it ships with ubuntu 16.04, so it does make trio easier to try for some people, (2) PyPy is still on 3.5 (though they're getting closer to 3.6), and twisted devs keep telling me that their users all deploy on PyPy. Plus it's much easier to compete with uvloop if we can cheat and use PyPy ;-).

[Fuyukai]

The tests don't pass because TrioDeprecationWarning is warned on test_locals; doing a with pytest.warns(TrioDeprecationWarning) fails also because it doesn't warn because of course.

[njsmith]

@Fuyukai Maybe try adding a recwarn fixture to the tests for the deprecated code, like in trio/tests/test_socket.py::test_deprecated_resolver_methods?

[Fuyukai]

Docs updated, task locals deprecated, and I rebased for a slightly cleaner commit tree.

[njsmith]

Doc build is failing with:

/home/travis/build/python-trio/trio/docs/source/reference-core.rst:1125:py:mod reference target not found: contextvars

because we're currently using the stable python 3 docs for intersphinx links, which means 3.6, and contextvars doesn't exist until 3.7.

I guess the slickest solution would be to use some intersphinx cleverness to direct just that link to the python 3.7 docs. Alternatively it wouldn't be the end of the world though if we dropped the link, or made it a regular URL link instead of an automagic intersphinx link.

[Fuyukai]

Given that it'll automatically fix itself in a few months time, I think the easier solution is to tell sphinx to ignore the warning and wait for the docs to point to 3.7 instead.

[Fuyukai]

RunVar support has been added. The build fails because the doc can't look up trio.TaskLocal (??).

Otherwise, ready for review.

[1st1]

I'm curious what's the motivation behind RunVar? Why ContextVar is not enough?

[1st1]

Right, but how is it different from a global/thread-local variable? Why does this specific use case deserve extra complexity?

[Fuyukai]

No clue. I was just asked to add it.

[njsmith]

@1st1 Another way to think of run-local data is that it's equivalent to attaching an attribute to an asyncio loop object, except that you can safely do it from third-party code. I agree that it's kind of a weird and obscure use case. But run_sync_in_worker_thread needs it to track some data to avoid spawning too many threads simultaneously (you can think of it as holding the run's thread pool), and run_sync_in_worker_thread is built using only public APIs, so we need something like this as a public API :-). (And the reason run_sync_in_worker_thread is written using public APIs is because as a general policy we try to do this whenever possible. This is like how when asyncio needs something from the Python core, then you don't add a secret API that only asyncio can use, you add a new public API and asyncio becomes one of the users of it. Features like run_sync_in_worker_thread ship bundled together with trio, just like asyncio ships with the interpreter, but we want to keep the door open for users to invent their own better replacements 5 years from now.)

I guess RunVar might also be useful for other kinds of "system service" code... for example, if we were trying to implement trio's version of call_soon_threadsafe using only public APIs, then you'd need RunVar (and also spawn_system_task, which is another weird and obscure API). The only reason call_soon_threadsafe has to be built in is that run needs to start it automatically on every call.

[Fuyukai]

The jenkins build hiccoughed on installing wheel it seems (??????) and I don't know how to resolve the doc error at all.

[njsmith]

Yeah, the jenkins build currently has an old pip that only speaks some ancient version of TLS, and right now PyPI is randomly breaking old TLS versions to encourage folks to upgrade before their CDN service forces them to turn it off permanently: https://status.python.org/ , https://pyfound.blogspot.com/2017/01/time-to-upgrade-your-python-tls-v12.html

So, it worked! I've pinged the guy who maintains that server about it.

The doc error is because the docs for RunLocal reference the docs for TaskLocal, and the PR removes the TaskLocal docs, so that reference doesn't work anymore. But, I've realized I was unclear about the big picture here – sorry about that! Let me try to explain the motivation for the RunVar thing.

In 0.3.0, we had TaskLocal, whose API was designed to be like threading.local but for tasks, and we had RunLocal, whose API was designed to be like threading.local, but for runs. This makes things easy, because once you know how to any of these you know how to use all of them. But now we're replacing TaskLocal with ContextVar, which ruins the symmetry. Solution: replace RunLocal with RunVar to keep things symmetric.

So getting back to the doc error: we should deprecate RunLocal and delete its docs entirely, which will certainly fix the error as a side-effect :-). Does that make sense?

I'll do a proper code review in a few minutes too.

[njsmith]

Looking pretty good! Two minor comments, and I also made a few in-place wording tweaks, so you'll want to pull before making more changes.

The main thing left is deprecating RunLocal, which is slightly more work than it sounds like, because there are some uses in _threads.py and _socket.py that need to be switched use RunVar instead. Do you feel like taking that on? I'm feeling bad that I ended up bait-and-switching you here, making adding RunVar sound like a smaller job than it is, so let me know if you want me to do the rest...

[njsmith]

I guess your editor is set up to automatically sort imports? I'm not going to stress over it here, but it does make it hard to tell what's changed... do you think you could get it to not do this in future PRs? (Or alternatively, if you absolutely can't stand unsorted imports, I guess you could set up isort alongside yapf so everyone does it? :-))

[Fuyukai]

Oh, this was an accident; I have PyCharm set to auto-optimize imports by default on my global scope which removes unused ones and re-arranges them into a different order. I forgot to turn it off for this project.

[njsmith]

Style nit: can you rename this test_runvar_basics or test_runvar_smoketest? I try to avoid using "sanity" in the jokey "not horribly broken" sense these days b/c the jokey sense seems to come from actually pretty gross stereotypes about mental health. (And same comment on the other uses of "sanity".)

[codecov-io]

Codecov Report

Merging #478 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
+ Coverage   99.26%   99.27%   +0.01%     
==========================================
  Files          89       89              
  Lines       10380    10542     +162     
  Branches      721      728       +7     
==========================================
+ Hits        10304    10466     +162     
  Misses         58       58              
  Partials       18       18

Impacted Files	Coverage Δ
trio/hazmat.py	100% <ø> (ø)	⬆️
trio/_core/tests/test_local.py	100% <100%> (ø)	⬆️
trio/_threads.py	100% <100%> (ø)	⬆️
trio/_core/_run.py	100% <100%> (ø)	⬆️
trio/_core/_local.py	100% <100%> (ø)	⬆️
trio/_core/tests/test_run.py	100% <100%> (ø)	⬆️
trio/_socket.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aee2676...1e21950. Read the comment docs.

[Fuyukai]

I made the requested changes, as well as fixing the RunVar export. The build is (still) failing because the changelog refers to RunLocal which doesn't exist in the docs anymore. (Seems like a problem that's gonna hit in the future too).

[njsmith]

@Fuyukai I did a last pass, and noticed a few more trivial things, so I just changed them :-). I think this is good to merge – want to double-check my changes and then click merge if you agree?

[njsmith]

Hooray! And thanks for your patience :-)