CodSpeed Performance Report

Merging #10425 will not alter performance

_{Comparing MarkusSintonen:improve-ns-handling-perf (3f129e7) with main (d587553)}

Summary

✅ 38 untouched benchmarks

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
annotated_handlers.py
dataclasses.py
fields.py
main.py
type_adapter.py
pydantic/_internal
_decorators.py
_fields.py
_generate_schema.py
_model_construction.py
_namespace_resolver.py
_schema_generation_shared.py
_typing_extra.py
_validate_call.py
_weak_ref.py
Project Total

This report was generated by python-coverage-comment-action

Thanks for the effort put into this. I haven't gone through all the changes yet, but wanted to communicate on a couple subjects we had with the team:

Skipping namespace fetching

Sydney had the idea a couple weeks ago if it was possible to skip namespace fetching logic entirely, if no stringified annotations are being used. If this is possible, I would really prefer going this way as it would most probably require less changes, and would improve performance even more.

Not long ago, I started exploring some solutions.

How Python evaluates stringified annotations

The (internal) typing._eval_type is used by Pydantic to evaluate types ¹.

For a given type, it iterates over the arguments (similar to your _needs_eval, more on that later) and calls ForwardRef._evaluate with the global and local ns.

To evaluate stringified annotations, ForwarRef calls the builtin eval function. So for a given model:

Forward = int

class Model(BaseModel):
    field: "Forward"

eval will be called with:

• source: "Forward"
• globals: sys.modules[Model.__module__].__dict__ ({"Forward": int, "Model": Model})
• locals: None

Using a lazy namespace

The idea was to use a lazy namespace implementation:

class LazyDict(dict[_KT, _VT]):
    def __init__(self, fetcher: Callable[[], dict[_KT, _VT]]) -> None:
        self.fetcher = fetcher

    @cached_property
    def data(self) -> dict[_KT, _VT]:
        return self.fetcher()

    def __getitem__(self, key: _KT) -> _VT:
        try:
            return self.data[key]
        except KeyError:
            # `eval` can inject `__builtins__`:
            return super().__getitem__(key)

In our case, fetcher could be something like (partial(merge_cls_and_parent_ns, cls=cls, parent_namespace=parent_namespace)).

This way, namespace logic is only run if necessary, and this also has the benefit of not letting any opportunity to mutate these namespaces dicts.

However, for performance and probably historical reasons, globals must be a dictionary (and not a subclass of dictionary, although passing LazyDict does not raise). __getitem__ is also not called at all, as internal C calls are being made for performance.

One idea we had yesterday was to not provide any value for the globals argument, and instead use the locals (which can be any mapping, and __getitem__ is guaranteed to be called). Our LazyDict.__getitem__ would then be implemented with the following steps:

1. Look into parent namespace, i.e. sys._getframe(x).f_locals
2. Look into the module namespace, i.e. sys.modules[cls.__module__].__dict__
3. Look into {cls.__name__: cls}

Or, if we want to be more generic, we could use a ChainMap of LazyDicts.

While this can feel a bit unusual (and I still have to confirm that the behavior is equivalent), this is probably going to be forward compatible as we will hopefully get python/cpython#121389 for Python 3.14.

Performance improvements

I did some tests locally on your branch. I first have a question regarding load times: on my end, it takes around 14s to load the k8s_v2.py file, and 13s on your branch. My guess is you are running on way better hardware than what I have (Ryzen 7 5825U), although the difference is quite huge. Could you confirm?

Regarding memory consumption, skipping namespace copies only reduces the number of temporary allocations, but we still ends up at ~600MiB when the module is fully loaded. While temporary allocations doesn't matter, I believe most of the time save is due to these reduced memory allocations, which is still a good thing.

Regarding the patch, I have mixed feelings:

• I definitely think our namespace management is still messy, and having this NsResolver class seems to help in centralizing this logic.
• I think this adds quite some complexity, especially regarding the relation between NsResolver and ImmutableNs (e.g. the chained globalns.resolve_namespace().unwrap_namespace() calls, NsResolver(ImmutableNs(dict(vars(base)))), etc).
• You added a _needs_eval function to skip type evaluation if not forward ref is available. We already have has_instance_in_type which account for more edge cases. If possible, I would avoid relying on this. This is essentially doing the same thing as _typing.eval_type but as you can see by comparing both implementations, it is done very differently ². This is also not really forward compatible, as Python may implement a new typing construct which requires special casing when trying to look for forwardrefs (e.g. the Callable special casing).

@Viicos

One idea we had yesterday was to not provide any value for the globals argument, and instead use the locals

I like this idea! Let me check it out. For locals we can indeed pass any Mapping implementation. But the globals is a weird one...

@Viicos

One idea we had yesterday was to not provide any value for the globals argument, and instead use the locals

I like this idea! Let me check it out. For locals we can indeed pass any Mapping implementation. But the globals is a weird one...

Great! I am pretty confident the ChainMap + LazyDict approach is pretty elegant (in the sense that it will be easy to understand + efficient) and can be implemented without too much trouble. Feel free to experiment with it.

@Viicos,

Thanks for laying out our most recent thoughts here. Looking forward to iterating with you folks to find the right solution here.

@Viicos I now pushed some WIP refactoring where globalns dict is not given at all and the names are resolved on demand from the chain of namespaces via locals arg. Its a bit more complicated than originally. Did not yet do other refactoring to just validate the idea. It almost works!

But...

There is 3 pretty complex tests that fail. (The one was my local issue)

┃  File                    ┃  Function                                                 ┃  Function Line  ┃  Error Line  ┃  Error      ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━┩
│  tests/test_docs.py      │  test_docs_examples[docs/concepts/experimental.md:46-84]  │  168            │  184         │  NameError  │
│  tests/test_generics.py  │  test_generic_recursive_models_triple                     │  1753           │  1754        │  KeyError   │
│  tests/test_generics.py  │  test_generic_recursive_models_complicated                │  1820           │  1829        │  KeyError   │
└──────────────────────────┴───────────────────────────────────────────────────────────┴─────────────────┴──────────────┴─────────────┘

Tried to dig a bit deeper but the tests are complex. Should try to isolate. But it might be that the change is revealing some hidden issue in the "experimental" feature or in generics handling. Those ForwardRefs handling in generics are super complex so its not obvious at all where its actually breaking.

My guess is you are running on way better hardware than what I have (Ryzen 7 5825U), although the difference is quite huge. Could you confirm?

Its MacBook Pro Nov 2023 - M3 Max

Oh hmm I might know why those generic forwardrefs tests fail. Its using some id(xxx) stuff in the 'schema_ref' things. But does the id(xxx) stay the same when doing multiple evals now without globalns and only with localns? No, see below

Oh hmm I might know why those generic forwardrefs tests fail. Its using some id(xxx) stuff in the 'schema_ref' things. But does the id(xxx) stay the same when doing multiple evals now without globalns and only with localns?

No this was not the case. Found a fix but not sure why this worked :D Doesnt seem to cause any regressions.

Now only the doc example experimental thing fails for some reason.

I think this adds quite some complexity, especially regarding the relation between NsResolver and ImmutableNs (e.g. the chained globalns.resolve_namespace().unwrap_namespace() calls, NsResolver(ImmutableNs(dict(vars(base)))), etc).

Refactored it now to drop the "extra layer" here. But, I would like to keep the namespace specific resolving in its own logic instead of using some generic class like LazyDict. I dont think trying to share the logic with other parts makes much sense as the needs are so specific for namespace handling. So proposed way would isolate some of the complexity in its own dedicated class.

We should probably be careful with precedence here - in dict updates, the latter dicts override duplicate keys in previous ones. We should follow the same pattern in NsResolver.

Following up with my commentary here. The original types namespace push logic was the following:

@contextmanager
    def push(self, for_type: type[Any]):
        types_namespace = _typing_extra.get_module_ns_of(for_type).copy()
        types_namespace.update(self.tail or {})
        self._types_namespace_stack.append(types_namespace)
        try:
            yield
        finally:
            self._types_namespace_stack.pop()

This, imo, is greatly flawed. Not to mention, this structure is already incredibly inefficient (which we're tackling in this PR, so not worried about that).

We're effectively analyzing a type in a new namespace (getting the ns associated with the type), but giving priority to the previous ns. This results in the following flawed behavior:

# test.py

from pydantic import BaseModel
from test1 import MyDataclass
from pydantic._internal._core_utils import pretty_print_core_schema

X = str

class Model(BaseModel):
    dc: MyDataclass

# test1.py
from dataclasses import dataclass

X = int

@dataclass
class MyDataclass:
    x: 'X'

The type of MyDataclass.x is evaluated to str because the test.py module namespace takes priority over that of test1.py. Thus, I do think it makes sense to move forward with a more intuitive ns precedence approach where we push to the top of a stack like structure, then search from the top down. Also makes stack management more efficient, as we can just push and pop on the end of the list.

Happy to help investigate why some tests fail when we make this change.

This, imo, is greatly flawed. Not to mention, this structure is already incredibly inefficient (which we're tackling in this PR, so not worried about that).

Yes, I think I understand what the problem is. It has been deprioritizing the pushed namespace because it's coming from the module level. So it's related to global namespace. If it's prioritizing it then the global namespace may incorrectly override a local namespace already added previously. That's why tests start to fail if its changed. So how I think this should be made more sensible is to have 2 namespace list in "NsResolver" to track globals and locals separate. So in this "push" we would append to global ns list. Then on resolving we just prioritize first locals and then globals. That would make things more clear hopefully as things are not so mixed up together.

Yes, I think I understand what the problem is.

This was indeed the issue. So separating the global and local namespace handling inside the resolver fixes the issue! So there was a old bug in the code. Added tests also against the revealed issue. Now the resolver separately manages the global namespace context and the chain of local namespaces. I think this makes the logic pretty clear as it no longer mixes and mangles the namespaces together but instead they are regarded as separate. cc @sydney-runkle nice finding!

The commit fixing this: ba78bd5

Hi @MarkusSintonen, thanks for getting back at this. Over the next three days I'll try to come up with a solution based on your work (there are still some issues with API design and some edge cases not covered by tests that are failing now -- that's why I want to take the time and be careful with the introduced changes). I'll also document everything because with all these changes, I expect issues to be raised once we release a new version.

I should be able to do it until Sunday evening.

This was indeed the issue. So separating the global and local namespace handling inside the resolver fixes the issue! So there was a old bug in the code. Added tests also against the revealed issue. Now the resolver separately manages the global namespace context and the chain of local namespaces. I think this makes the logic pretty clear as it no longer mixes and mangles the namespaces together but instead they are regarded as separate.

Fantastic - I think it makes sense to keep these compartmentalized. Much more intuitive, as well.

I'll do a thorough review once @Viicos comes back with findings re the lazy namespace evals.

Great work here folks. Really looking forward to having this new approach.

Ok so I had the opportunity to take a deeper look at how namespaces are used throughout the code base. First, let's get back at the issue with the namespace push logic, as described in #10425 (comment):

While it is indeed flawed as shown in the given example, it was necessary as ~50 tests are currently failing on main when commenting out the types_namespace.update(self.tail or {}) line. Now the thing is we know (bc tests are passing here) that in the end — when the namespace handling cleaning will be done — the merging operation won't be necessary. But we need to be careful, as tests may not cover every case.

We may be wondering: if the merging operation isn't necessary, is it necessary to keep this types namespace stack approach? The answer is "it seems to depend". Let's say we are generating a schema for a named tuple:

from typing import NamedTuple

from pydantic import TypeAdapter

class NT(NamedTuple):
    f: 'Forward'

TypeAdapter(NT)

To do so, we push the NT class to the stack, and evaluate annotations with the global namespace of NT (the module ns). So instead, we could do something like:

diff --git a/pydantic/_internal/_generate_schema.py b/pydantic/_internal/_generate_schema.py
index c161818e3..15404c5df 100644
--- a/pydantic/_internal/_generate_schema.py
+++ b/pydantic/_internal/_generate_schema.py
@@ -1547,8 +1547,7 @@ class GenerateSchema:
             if origin is not None:
                 namedtuple_cls = origin
 
-            with self._types_namespace_stack.push(namedtuple_cls):
-                annotations: dict[str, Any] = get_cls_type_hints_lenient(namedtuple_cls, self._types_namespace)
+            annotations: dict[str, Any] = get_cls_type_hints_lenient(namedtuple_cls, get_module_ns_of(namedtuple_cls))

but the thing is for each field of the named tuple, _generate_inner will eventually be called, and forward refs may need to be evaluated (and this will used _types_namespace property, depending on the namespace stack). Of course in this example, we know for sure forward refs will not be evaluated again if they failed to do so when first using get_cls_type_hints_lenient, but there are cases where it can. Anyway, this means the type namespace stack pattern will be hard to remove entirely. But because the merging operation won't be necessary, it can be simplified/renamed to be a types stack:

class TypesStack:
    _types: list[type[Any]]

    @contextmanager
    def push(self, typ: type[Any]):
        self._types.append(typ)
        try:
            yield
        finally:
            self._types.pop()

class GenerateSchema:
    ...

    @property
    def _types_namespace(self):
        return get_module_ns_of(self._types_stack.tail)

The reason I'm mentioning this is you moved the stack logic in the NsResolver class (alongside with the weakdict logic) and I don't think it is a good idea. The stack is pattern that only belongs to the GenerateSchema class, and mixing this logic in the NsResolver class makes things hard to follow (lots of methods and chained calls).

For reference, here is a list of test cases I'm considering fixing (or documenting as a limitation if not possible). I'm going to work on it this week so no need to duplicate our efforts.

@Viicos,

Great digging. The failing tests are super helpful here.

Let me know if I've missed anything:

• The namespace merging that we do currently is a) wrong and b) not necessary, if we want correct behavior.
• We do need to keep track of a types stack in order to access the appropriate namespace for type resolution
• I think it makes sense to keep the types stack in the scope of the generate schema logic
• I'm definitely in favor of retaining some of the NsResolver and ImmutableNs logic from a helpful abstraction / organizational perspective
• I think it could be good to address a few of these concerns (ex, the problematic ns merging) in small, separate PRs for ease of review + tracking down issues

Excited to make some more progress on this front in the coming weeks. Thanks folks!

@MarkusSintonen,

quick update on the work we've been doing. While I first thought we could be smart by only passing a local namespace to the eval function, it turns out it wasn't playing well with built-ins (you can try running the k8s_v2.py file with the future import and see that it fails) and the performance benefit wouldn't be that huge anyway. Sorry about that.

So I decided to try implementing things by always coupling the locals and globals as a named tuple, so that it can be easily unpacked when calling eval functions. In most cases, the globals will always be the module's __dict__ and no copy will be performed. For locals (which can come from different sources), I implemented a small lazy dict data structure that will merge the local ns together only if eval calls __getitem__ on it.

But most importantly, we decided to completely specify how we handle namespaces for type evaluation, in particular when doing rebuilds. The document (which will be included in docs at some point) can be found here.

My WIP implementation can be found on this branch: ns-refactor. It also cleans up the handling of dataclasses and misc. things in the GenerateSchema class.

Going to close this in favor of #10530. Thanks so much @MarkusSintonen for kickstarting this process!