Use a context variable to pass Schema context

[lafrech]

Implements #1826.

Hopefully solves/fixes/replaces/obsoletes #1791, #1833, #1700, #2173, #1617, #1825.

I think most current use cases are covered.

Context can be passed in dump/load call or set in a context manager. The former overrides the latter.

Things can be renamed if CONTEXT and Context are not specific enough.

context used to be a dict but can now be anything the user wants. I didn't see any reason to enforce a dict. We could let CONTEXT default to an empty dict (immutable, to avoid issues) but since the type may be anything I didn't do it.

To access the context from a function, one must use CONTEXT.get(). This may look a little less nice than before (self.context) but people familiar with context variables shouldn't frown upon that. We could make it available from a schema or field property, but it would hide the technical implementation a little too much IMHO and could lead to misunderstandings.

I should add tests showing it works from any hook.

Docs need to be updated but I'd rather wait for feedback before I do it.

[lafrech]

Thinking of it, this PR does either too much or not enough.

If we kept only the explicit context manager form, this could be achieved in user code. The added value in providing this from marshmallow code is to allow passing the context in the load/dump call rather than having to call the context manager. And of course to avoid users recreating the wheel on each project.

We could remove the whole feature.

Or we could go a bit further and expose the context with schema or field properties to hide the context variable. I just pushed a new commit doing this. Function field is a special case as it cannot access the field or schema, so I reintroduced the 3.0 logic.

Overall, I prefer that latter option to the full removal, at least for now. It makes for a smoother transition.

The context feature was introduced when context variables where not a thing and IIUC with uses cases in mind that were related to Function and Method fields, although users have found other ways to use it since then. BTW, I think these fields introduce too much complexity in the code and I'd like to remove/replace them (#2039).

I didn't check but I don't think the performance overhead is significant. If it was, we could publish this in a ContextSchema subclass.

I'd like to go on with this.

• Decide whether or not the context should be a dict and if so provide an empty (ideally immutable) dict as default value
• Add tests showing how it works with hooks
• Update docs

[lafrech]

We can provide an empty mapping as default:

DEFAULT: MappingProxyType = MappingProxyType({})

CONTEXT = contextvars.ContextVar("context", default=DEFAULT)

Then, there is always a context, be it empty, so the test in Function field doesn't work (and the code will crash later if context is needed).

The point is do we want to distinguish no context and empty context?

It might be wiser not to trade semantics for ease of use and keep None as default (or don't even set None as default and use a try/catch instead but the perf impact might be negligible). This allows users to really set any value as context.

Since my latest commit, the context variable is only accessed directly in our code, users access it via schema/field properties, so we can wrap the access with whatever code we want, it doesn't have to be a one-liner.

[sloria]

i'm on board with replacing the current marshmallow implementation of "context" with a contextvars-based API, so i'm good with moving forward with this feature 👍 so it's just little API decisions to make from this point.

Decide whether or not the context should be a dict and if so provide an empty (ideally immutable) dict as default value

imo we should let the user decide which data structure to use for their context

It might be wiser not to trade semantics for ease of use and keep None as default (or don't even set None as default and use a try/catch instead but the perf impact might be negligible). This allows users to really set any value as context.

might make sense to just not set a default. Function already fails loudly if the passed function expects context but it isn't set, so there's an opportunity to simplify the code if we let ContextVar.get() to raise the error.

[lafrech]

I applied said changes.

I removed the default. I don't mind Function field not catching the error about no context. In fact, this shouldn't deserve a ValidationError. The error lies in the code, not in the input.

However, I find it less convenient to use:

                # With None as default
                if (context := Context.get()) is not None:
                    val *= context.get("factor", 1)

                # Without default
                try:
                    context = Context.get()
                except LookupError:
                    pass
                else:
                    val *= context.get("factor", 1)

The first form makes it nicer when using the context in a function that is meant to work either with or without.

I have never been using contexts. Maybe functions are meant to work only one way and it is up to the user to provide a context on each call, even a default one. Makes sense. If it is the way you see it, I'll leave it as is and just remove the part of the test that does what I copied above. I agree it means even less API surface.

I still need to add tests checking the use in hooks.

[sloria]

However, I find it less convenient to use...

i suppose we could allow optionally passing a default to Context.get

from marshmallow import Context 


Context.get() # => LookupError
Context.get(None) # => None

[lafrech]

Good idea. I tried that.

I had to create a singleton to distinguish None from no value. I clumsily copied missing.

Pandas does it differently using an Enum.

[sloria]

Note that if we go this way, the only added value of this is to provide the context variable and manager, but technically, this could be left to the user, since it is not called anywhere in the core. I still think it is nice to provide, if only to offer a clear migration path.

i see what you mean. i'm torn on this. on one hand, the current context implementation has rough edges that contextvars obviates. on the other hand, i don't want code ourselves in a corner with an underpowered API.

one incremental approach would be to deprecate the current context implementation for 4.0 with a warning that recommends using contextvars and leave it up to users to handle context on their own (we could even provide some guidance in the Upgrading Guide docs).

we'd postpone the addition of a convenience API for a future release

[lafrech]

We did have a little adherence in Function field where Context is called. I just removed it, which is a nice cleanup. It is now up to the user to define a function that gets the context from Context. Context code is now independent from the core.

This feature is something I had in mind for a while so I'd be happy if it made it through, but that's not a good reason to merge.

A better reason is the cleanup of the existing feature. But in fact, I wouldn't mind removing the current feature (rather than deprecating) even if we only provide Context as a doc hint. The point is that context vars make the original feature not so useful.

I have no idea how widely the feature is used.

[sloria]

ok, i'm good with removing the current context implementation for 4.0 and documenting how to use contextvars in the docs.

we could even add the Context implementation as provisional/experimental API (e.g. put it in marshmallow.experimental.context).

[lafrech]

we could even add the Context implementation as provisional/experimental API (e.g. put it in marshmallow.experimental.context).

I can do that. Is it any different than a docstring in the top of the file stating it is experimental? What would be the exact meaning? Expressing the fact that next major may ship a modified version or remove the feature altogether?

[sloria]

putting it in experimental makes it clear to users that they're using unstable API even if they don't read the docs, e.g. if they discover it through autocomplete/introspection. it's a pattern i've seen in other libraries like scikit-learn and strawberry.

What would be the exact meaning? Expressing the fact that next major may ship a modified version or remove the feature altogether?

i would treat it like pre-1.0 software, i.e. breaking changes could be made with minor releases. here's the warning in scikit-learn's docs

.. warning::

    The features and estimators that are experimental aren't subject to
    deprecation cycles. Use them at your own risks!

[lafrech]

OK, I put it in experimental.

• Add tests demonstrating the use with hooks
• Changelog
• Documentation
• Dostrings (versionchanged for Schema, Function and Method)

[sloria]

looks great. just a few small things. go ahead and merge after those

[sloria]

looks great! 🚢 🇮🇹 go ahead and merge unless you have any further changes to make

[sloria]

thought(non-blocking): should we replace this with the equivalent using the new Context API?

[sloria]

#2727 addresses this