Enabling thread_inherit_context and context_aware_warnings by default on both builds

Core Development
21

That benefit only exists if library authors who are aware enough of the problem and affected by it are only going to use the new API and not handle older supported Python versions.

That seems like a subset of a subset of a subset, and I don’t see a reason why a library author would switch to this API and not support prior python versions, when flagging this for users to just set -X thread_inherit_context=1 works without multiple code changes, and requires the same level of awareness from library authors to accomplish.

22

As a user of such a library, having to set global state like that (and review whether doing so would affect other libraries I might use) would be a problem. Maybe enough to persuade me not to use the library. Having a workaround in that case would allow me to use the library 2 or 3 Python releases sooner, which to me could be important. It’s still extra work for the library author, sure, but isn’t that true whenever you support a group of users with unusual needs?

2 Likes
23

As a library author, I wouldn’t use this API is the point I’m getting at. It doesn’t allow me to actually solve the issue for all supported python versions for users, at the laziest, I’m still having to tell python 3.14 users to set the variable, and for 3.13 completely unsupport the use.

Or… I can design it to work properly across all version with code that works all the way back to python 3.7 without the new API so long as I write my library code aware of this.

The extra work the library authors already have to do to not have broken users here is why this API doesn’t make sense, it’s only libraries willing to push the problem onto users to move to the newest python that benefit from this.

24

Ah, OK. I misunderstood. My impression was that the only way to make code that worked on all Python versions was to require users to set the global flag.

1 Like
25

Yes, that’s my understanding as well. In the absence of a new API, there’s no current mechanism that plays nice with both async context variables and threads, other than globally changing the default behaviour.

Hence why I think a new API would be worth it, as it gives library authors the choice between saying to their users “just flip the global setting yourself until the actual default changes” or instead saying “in 3.15+, it will just work, in earlier versions, you’ll need to flip this global setting”.

2 Likes
26

Mine as well. I’m not sure what solution @mikeshardmind is getting at.

For NumPy, keeping the state global is not a reasonable option. IMO moving the state to context variables is worth the thread and async-safety despite the wrinkle around threads on the GIL-enabled build. However, having a way to indicate to the interpreter that a variable should be inherited by newly spawned threads completely solves the problem users run into without needing to educate them about the startup option.

The fact that the fix can’t be backported to old Python versions is too bad, but I’d still take Python giving us a lever to use in new versions.

1 Like
27

I haven’t followed the technical back and forth, but I do maintain libraries that rely on contextvars and threads. Flask (sync) and Quart (async) use a contextvar to store the “current request” for each worker (thread or coroutine). Both also accept sync or async callbacks in various places, or users may want to perform some work in a background thread/coroutine. We (and Django) use asgiref SyncToAsync and AsyncToSync wrappers to ensure the correct type will be used throughout the pipeline of request handlers and callbacks. Asgiref does a huge amount to ensure it’s consistent across threads and coroutines. It already works with thread_inherit_context: asgiref/asgiref/local.py at a7cc976fe8a2395307834bc4aebb462b9f379d55 · django/asgiref · GitHub

I can point the maintainers of asgiref at this discussion, but I’m very sure they’d be ok with the proposed new API as well. Presumably, Flask would pass the arg to its contextvar, and asgiref would detect that it’s set and keep doing the right thing.

28

Okay, I guess I should more fully explain.

contextvars have thread local documented behavior from day 1 here. That behavior was clear, though surprising to those who hadn’t read the docs or implementation. The free-threading behavior being different is surprising to those who wrote their code knowing the prior behavior.

I think everyone here agrees it wasn’t the right choice, but changing this behavior isn’t a fix, it’s a behavior change that we believe results in it being a more useful tool.

Those already using it knowing it was thread-local have working code that might be broken by changing this, Those whose code never hit cases where it being thread-local mattered have working code that should tolerate this being changed.

The problem is that in 3.14 we went from “This is the behavior” to “This is the default, and it might change under you with an interpreter setting”, so while library authors can shrug if they want and say “you set something that breaks the documented behavior”, that makes the setting a no-go as a real solution here. It also means that as of python 3.14, especially for libraries that want to support both the GIL-enabled and free-threading, libraries already need to behave correctly under both cases.[1]

One of the parts I take some issue with is this is proposed as a temporary transition API.

If we’re able to acknowledge that both behaviors have valid uses, then this should probably just be a planned permanent API, not one that’s already being seen as something to deprecate in the future.

If we’re taking the stance that context-local should always include thread spawning, and that it not being this way a mistake that we don’t care that people unfortunately relied on, we don’t need this API, we can set a date for when users can expect this to switch, but until that point in time, libaries shouldn’t be relying on contextvars to handle this without some wrapping on their part.

It’s not clear to me which of these stances on behavior everyone here thinks is the case, but this API being added with a plan to deprecate it seems to be getting the worst of both of those stances.

I’m not going to adopt an already planned-to-be-deprecated API if I already have to work around this before the version it’s available.

If both behaviors are valid then this shouldn’t be deprecated. This outcome is fine with me, the API becomes adoptable when 3.15/3.16 is my minimum, and it’s nicer than the wrapping currently required.

If it’s intentional to on the long-term view invalidate the previous documented behavior even with code that relies on it, that makes the API more of a nuisance than a help, because it’s going to result in multiple stages of code churn for a moving target in python’s behavior. Even if the wrapping without the API is more annoying, it’s also going to be required to stick around after the date of the deprecation if libraries want the previous behavior (the one they in theory were designed for)

  1. There are cases where “behave correctly” can legitimately mean to allow divergence but toss a note in the docs and let callers of the library structure their code to work in either case. I believe this is the route NumPy effectively went, and I don’t see that as something that requires anything else of NumPy. ↩︎

1 Like
29

Thanks for the mention @davidism.

Yes, so it’s a long thread, and I may have gone a little too fast but… :sweat_smile:

For asgiref:

The inheritance on the free-threading builds was the surprise for us (as a new behaviour) but we already worked around it. Regardless of the state of thread_inherit_context we ignore inherited data unless asgiref itself re-homed it, so changing the default won’t affect us.

I think the warning would be quite annoying. It would be a false positive for us, and Django and Flask users would report issues about it, and there would be little we could do. (I think it might be trying to detect more than is realistic.)

The transitional API doesn’t affect us. (We don’t need it.) I glazed over some of the discussion there so I defer to those who know more.

Thanks!

3 Likes
30

Thank you all for finding real-world examples where turning on the warning will cause pain. I now think we shouldn’t warn about this in 3.15 and am no longer advocating for that.

In addition to django/asgiref, this will also cause warnings in sentry-sdk and OpenTelemetry.

So I think Neil’s thread_inheritable idea needs to become a three-way option with the following behaviors:

  • True: Newly spawned threads capture the context.
  • None: Threads start with no inherited context. A future Python version may warn about the thread_inherit_context startup option changing default values in the future.
  • False: Threads start with no inherited context. A future Python version will never warn about this.

NumPy can then use thread_inheritable=True and other libraries like asgiref, django, sentry-sdk, and OpenTelemetry can have the entire 3.16 release cycle to make sure their ContextVars are created with thread_inheritable=False.

1 Like
Make `ThreadPoolExecutor` propagate context at `submit()` time
31

I’m not sure I’ve folllowed. (Sorry).

I thought the argument was that inherited context was the future, the way to go. We would just switch it but there’s the backward compatibility concern.

The start up option means we need to be able to handle either case already.

We already fixed it. We do a lot of work to make sure we handle either case. But then this:

That makes it sound like we have more work to do going forward? (That sounds bad)

It feels like a lot of churn. (Remember we have to support five versions of Python at the same time here. We can’t easily start using a new keyword for many years yet.)

I guess as a library maintainer my ask is that you get 100% clear on the end goal and then head there by the most direct route. We can adjust, but multiple adjustments are a real pain.

2 Likes
32

I think given the number of projects we’ve found where even adding a new warning would be disruptive, I think we need a more considered approach and “just changing it” won’t work. I do still think enabling a warning, eventually, is a good idea, if we do decide to ever change the default on the GIL-enabled build.

I hear this loud and clear. In order to get to a version of the GIL-enabled build with thread_inherit_context enabled by default, I think we need to generate a warning. We’d like to give libraries a way to opt out of the warning via the keyword argument in gh-154564.

I could also give up on the idea of changing the default on the GIL-enabled build. In that case there’s no need to generate a warning and no need for asgiref to make a code change to bypass hitting the warning.

That said, I don’t think it would be a large code change, something like this:

if sys.version_info >= (3, 16):
    new_context_var = functools.partial(ContextVar, thread_inehritable=False)
else:
    new_context_var = ContextVar

I’ll probably propose adding something like this for NumPy’s uses of context variables with thread_inheritable=True. I’m personally fine with having different behavior on different Python versions for NumPy’s uses of context variables.

33

It feels like the startup option needs to be removed too. If inherit is to be the correct behaviour, and individual call sites are to be given a kwarg to enforce that (early) then the start up option needs to not override that for the entire interpreter.

As it stands, libraries still need to be able to handle either behaviour, and that’s already the case on 3.14. (And was a new requirement there, perhaps not intended?)

The discussion then amounts to remove the old behaviour entirely.

I can’t say how much of a migration that will require, over the ecosystem.

34

There are use cases for not inheriting as well. I think the answer is actually to keep the documented behavior, remove the interpreter option, and change how we’re thinking about this from being configured application-wide or per-variable (which may also be incorrect) to configuring it per concurrency abstraction, as that’s the level of design where you can actually determine whether it makes sense for context to be inherited, or if the point is to launch something that isn’t intended to be in the same context.

1 Like
35

Just a short update on this. We are not planning to change anything about this in 3.15. The plan is to start fixing this in 3.16. The details of that are to be determined and feedback from library authors and other stakeholders would be appreciated. The provisional plan is:

  • add a per-context-var flag that allows it to be explicitly thread-inherited or not. When this flag is set, the thread_inherit_context global would be ignored.
  • add a warning for contextvars that don’t have the flag set and would end up inheriting if we change the default of the thread_inherit_context flag. The goal is to warn only in the cases where toggling the default value of the flag actually matters. This warning would be added after 3.16.
  • eventually (could be a number of releases, I don’t think we need to rush it), remove the thread_inherit_context option and make it effectively true. The consensus seems to be this is the better default and what most contextvar users want.

If I had a time machine like Guido’s, I’d go back, remove the thread_inherit_context option and add the per-variable flag instead. Then, the context aware warnings could use a per-variable flag, rather than having a global flag that differs between the default and free-threaded builds. Alas, past me was not smart enough to think of that.

1 Like
36

I still think this is the wrong place to do it. Placing this on threading.Thread, as well as abstractions ontop of threading (like executors) places it where the decision should be made on “is my use of concurrency here intended as being in the same context, or am I spawning a thread for some sort of logical seperation from the prior context”.

3 Likes
37

I think that a warning should be added to runtime (in a minimally-disruptive way per comments above) and documentation should be clarified in the stdlib.

Specifically, I and many colleagues* thought thread_inherit_context=0 was the intended purpose of contextvars until reading this issue! Our assumption, supported by the bad default, was that contextvars’ general behavior was threading.local() plus automatic context copying/localization in async tasks plus the user-facing API in contextvars.

In other words, we were deliberately reaching for contextvars as a “better threading.local()”, not an inheriting/forking context.

Now that I’ve read this issue and realize that the thread-localization behavior was always an accident (and likely lacking properties that an actual threading.local() would provide), I agree that this bad default should be fixed everywhere.

But I think the warning should be quite loud! This is because there’s broken code in both directions (people with bugs caused by context not cascading across threads, and my cohort’s “people who expected it to not cascade”), so there’s no risk-free approach here; even doing nothing would continue causing bugs due to the propagation inconsistency.

It’s hopefully a bullet worth biting, since there are probably more existing broken behaviors resolved than new ones caused by this change, but will be surprising to some!

Before releasing the change, it would be great if:

  1. Release notes loudly warn about this.
  2. warnings warn about this at runtime (even if disruptive, the risk of silently supplying unexpected values via thread_inherit_context is worse).
  3. thread_inherit_context and context_aware_warnings should be referenced in the documentation of contextvars.
  4. Ideally, the documentation of contextvars should loudly/above-the-fold mention the list of situations in which context is copied or created afresh in the stdlib (it’s only a three item list: new threads, new asyncio contexts, direct user manipulation of context). Pedantically this note might not belong in contextvars (threads/async are just users of that module), but practically I think it would help avoid mistaken assumptions like the one I held (don’t ask for how long…).

*highly informal poll w/ N=9. 6 had this belief; 3 either didn’t know or understood the actual existing behavior.

38

Separately from my above comment, I agree with @mikeshardmind’s point: setting a thread-locality bit on ContextVar itself is the wrong place to do it.

I don’t think making ContextVar itself aware of thread-cascading behavior via a kwarg makes sense. It’s confusing to reason about, since it’s fundamentally a use-case-agnostic library configuring behaviors for a specific usage of that library by other code. I think that clutters the API, and would give rise to subsequent requests like ContextVar(asyncio_inherit_context=True/False) or ContextVar(subinterpreter_inherit_context=True/False), or mutating the value of thread_inheritable after the fact (e.g. for spawner threads whose purpose is to create other worker threads).

Even if thread_inheritable were added, user code would still need to be updated to be forwards or backwards compatible with it and/or the change to thread_inherit_context.

There are three situations where forcing thread_inherit_context=0 behavior might be desirable, but each one should be handled thus:

  1. Users who control the code that creates threads. Those users should explicitly configure the context when starting the thread a la Thread(..., context=Context() / copy_context()). Stdlib APIs that themselves spawn threads (e.g. ThreadPoolExecutor and things that wrap it) should gain configuration options that control the context that is passsed to new Thread objects if they don’t have that already.
  2. Users who do not control the code that spawns the threads their code resides in, but control the ContextVars they’re interested in. Those users can make their code proof against thread_inherit_context=1 by making their ContextVars thread-aware. This might look, roughly, like:
    a. Replacing ContextVar instance defaults with 2-tuples of a sentinel and the default value.
    b. Replacing code like var.set(foo) with var.set((current_thread(), foo))
    c. Handling the 2-tuple appropriately where var.get is called (e.g. resetting on thread mismatch).
  3. Users who control neither what thread their code is running in nor the contextvars (e.g. they’re using a contextvar set by a library, and running on threads spawned by another library which does not allow explicit configuration of context-cascading behavior). These users’ code will break, but this situation is hopefully both rare and will be made even rarer by the presence of docs/runtime warnings per my comment right above this one.

I suspect that the more that ContextVar can be a “dumb bag of data” whose control semantics are left up to harnesses (thread creators, async runners), the easier its internals will be to maintain, and the easier it’ll be for users to understand.