Add contextvars support. #478
Conversation
|
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
I guess we'll also want to replace @1st1 would you have any objection to is releasing a |
|
Would you mind to wait a couple of days for this? I will try to find time to do this. |
|
Here we go: https://pypi.python.org/pypi/pep567/0.1.0 |
|
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. |
|
Huh, I figured we could just call the module |
|
"contextvars" name is taken on PyPI. |
|
Heh, whoops. Though, the |
Yeah, but that can be really confusing to some users (I'd be confused).
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... |
|
@1st1 if it were just trio/asyncio/... who had to notice this I wouldn't care, but everyone who wants to use a |
|
Sure, please do. I'd be totally OK to rename the package. |
|
Email sent! |
|
Thanks! :) |
|
@Fuyukai If you want to keep making progress on this while we're waiting to hear back, I guess you can start with using You'll want to add an entry like: ":python_version < '3.7'": ["pep567"]to the We will want to deprecate
Also, do you want to handle the analogous (And thanks for working on this btw!) |
TIL this is a thing. But sure, I'll do those changes. |
|
Alright, thanks to Nathaniel, the backport is now available under "contextvars" name :) So please update the dependency in your PR to "contextvars". |
|
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... :-) |
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. |
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?) |
|
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.)
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 ;-). |
|
The tests don't pass because TrioDeprecationWarning is warned on test_locals; doing a |
|
@Fuyukai Maybe try adding a |
|
Docs updated, task locals deprecated, and I rebased for a slightly cleaner commit tree. |
|
Doc build is failing with: because we're currently using the stable python 3 docs for intersphinx links, which means 3.6, and 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. |
|
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. |
|
RunVar support has been added. The build fails because the doc can't look up trio.TaskLocal (??). Otherwise, ready for review. |
|
I'm curious what's the motivation behind |
|
Right, but how is it different from a global/thread-local variable? Why does this specific use case deserve extra complexity? |
|
No clue. I was just asked to add it. |
|
@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 I guess |
|
The jenkins build hiccoughed on installing wheel it seems (??????) and I don't know how to resolve the doc error at all. |
|
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 In 0.3.0, we had So getting back to the doc error: we should deprecate I'll do a proper code review in a few minutes too. |
There was a problem hiding this comment.
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...
| ) | ||
| from . import _public | ||
| from .. import _core | ||
| from .._util import acontextmanager |
There was a problem hiding this comment.
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? :-))
There was a problem hiding this comment.
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.
trio/_core/tests/test_local.py
Outdated
|
|
||
|
|
||
| # scary runvar tests | ||
| def test_runvar_sanity(): |
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
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). |
This doesn't change anything because we always initialized it anyway, but it's slightly cleaner.
|
@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? |
|
Hooray! And thanks for your patience :-) |
This adds PEP 567 contextvars support to the core of Trio.
Todo:
Closes #420 and closes #417, see also: #178.