Replace string nodeids with a structured NodeId internally

[nicoddemus]

pytest identified every collection-tree node with a plain "::"-joined nodeid string, repeatedly re-parsed (split/partition) at dozens of call sites for cache persistence, terminal/JUnit reporting, and stepwise/ subtests bookkeeping. Introduce a NodeId dataclass (src/_pytest/_nodeid.py) and use it internally as the dict/set key and equality type everywhere a nodeid was previously compared or looked up as a string, while keeping the existing nodeid: str property as a full backward-compatible surface for external plugins.

Function's NodeId also carries structured per-parametrize-call data (ParamId: id/argnames/scope) when built from live collection data, laying groundwork for future scope-aware scheduling (e.g. in pytest-xdist) without committing any consumer to it yet.

[RonnyPfannschmidt]

this is shaping up nicely

we'll have to investigate how to deal with the loadscope groups xdist tucks into node ids at the moment as thats a mess that messes with the strings

[nicoddemus]

Should we expose the new node id types as public? I wonder if people will need at least for type annotations...

[The-Compiler]

Really nice cleanup!

[bluetech]

Interesting work!

pytest identified every collection-tree node with a plain "::"-joined nodeid string, repeatedly re-parsed (split/partition) at dozens of call sites for cache persistence, terminal/JUnit reporting, and stepwise/ subtests bookkeeping.

Is the rationale here performance? If so, is there a way to show it is indeed faster?

Function's NodeId also carries structured per-parametrize-call data (ParamId: id/argnames/scope) when built from live collection data, laying groundwork for future scope-aware scheduling (e.g. in pytest-xdist) without committing any consumer to it yet.

I don't understand what this means, can you expand a bit? I think this explains why ParamId contains argnames and scope, which is otherwise unexpected since they're not present in the string nodeid and are conceptually redundant, so I'd like to understand it better.

[bluetech]

Hmm is there code that mutates report.nodeid?

[nicoddemus]

Not internally, but there might plugins out there that do it, so I decided to just implement a wrapper and convert to a OpaqueNodeId internally.

Alternatively, we can use the setter to raise an error instead.

[nicoddemus]

Is the rationale here performance? If so, is there a way to show it is indeed faster?

No, AFAIU we always found node ids to be a bit hacky, because while the original idea was to produce an opaque unique id for test items, in the end we always end up parsing it to extract information (such as the path to the file relative to the root), so it makes sense architecturally to use a data structure for it, with the information plainly available.

I don't understand what this means, can you expand a bit? I think this explains why ParamId contains argnames and scope, which is otherwise unexpected since they're not present in the string nodeid and are conceptually redundant, so I'd like to understand it better.

@RonnyPfannschmidt might elaborate a bit given he asked to add this information, but I assume is that given we can now have a richer data structure, we might as well store more information on it.

[RonnyPfannschmidt]

there are about 3 key details that always created a headache

a) we split node-ids all over the place in absolutely hackish ways - this has been a recurring source of grave errors
b) in mutliple venues of communications we ended up with nodeids that are broken in some ways (one of the msot daunting examples being loadscope groups tacking strings on front of nodeids which in turn breaks consistent reporting
c) parameters in nodeids where always completely lossy - now we have a bare minimum of metadata to make sense of/order nodes

and then there's a number of other issues relating to that - so this really is about removing a mess and pain

[nicoddemus]

@RonnyPfannschmidt @bluetech @The-Compiler please take another look. 👍

[bluetech]

Thanks for the update.

Some more comments/replies:

---

@nicoddemus said:

No, AFAIU we always found node ids to be a bit hacky, because while the original idea was to produce an opaque unique id for test items, in the end we always end up parsing it to extract information (such as the path to the file relative to the root), so it makes sense architecturally to use a data structure for it, with the information plainly available.

Sure. However, I think the aim should be to reduce use of node IDs to a minimum in favor of using Node directly. For example we did it recently with the matchfactories change. That's better because the Node is the ultimate source of truth. That doesn't necessarily conflict with this work, but it may affect the consideration of the tradeoff between extra complexity and benefit.

---

@RonnyPfannschmidt said:

a) we split node-ids all over the place in absolutely hackish ways - this has been a recurring source of grave errors

BTW regarding this, if I search for "::" under src/ I see several cases of parsing remaining. It would be good to convert them in this PR so we can better evaluate the benefit.

---

@RonnyPfannschmidt said:

b) in mutliple venues of communications we ended up with nodeids that are broken in some ways (one of the msot daunting examples being loadscope groups tacking strings on front of nodeids which in turn breaks consistent reporting

Do you have a reference for this? I'd like to understand this better, and how structured node IDs would help.

---

@nicoddemus said:

@RonnyPfannschmidt might elaborate a bit given he asked to add this information, but I assume is that given we can now have a richer data structure, we might as well store more information on it.

@RonnyPfannschmidt said:

c) parameters in nodeids where always completely lossy - now we have a bare minimum of metadata to make sense of/order nodes

Is this referring to use of nodeids intra-process or inter-process (serialized)?

If it's intra-process, as discussed above, can we replace the use of nodeid with Node?

If inter-process, is the idea to enrich the serialization from string to object?

---

Regarding OpaqueNodeId:

I was thinking whether there's any reason not to make it a typing.NewType instead of the wrapper type. That breaks down into whether there's a need to access path and rest and not just the __str__. I see there is one use of path in cacheprovider. And that makes me think, maybe it's better to change the cacheprovider serialization from string nodeid to structured item/collector node ID, then this one use of opaque is eliminated.

Furthermore, it just generally seems our aim should be to eliminate use of opaque in general, so I looked at a few of the uses.

In stepwise the situation seems similar to cacheprovider. If we serialize the structured node ID then I think we can avoid opaque here.

In junitxml I see a use of as_opaque, but I don't really follow why it's needed. If I comment out self.id = id in _NodeReporter.__init__ all junitxml tests still pass, so it looks like it's unused.

In terminal there are a lot of uses. I think the root cause of this is the use of string node ID in the pytest_runtest_logstart and pytest_runtest_logfinish hooks. Which brings up the point of how we can avoid losing the structured context here.

[bluetech]

I wonder if we can use item.path.relative_to(item.config.rootpath) here instead, thus eliminating one nodeid use?

[RonnyPfannschmidt]

im of the impression that the file collector id should be used there instead of a path

[RonnyPfannschmidt]

Opus found a conflict between this and the FunctionDefinition collection node work (#14769, follow-up #14805): Function.__init__ here drops the nodeid parameter, which that work relies on to keep item ids flat when a definition node is interposed. git merge-tree confirms the branches conflict in python.py.

The underlying point: leaf creation should not add a name. An item isn't a new name segment, it's a call with parameters registered against an already-named collector node — the function definition. So leaf takes only the params and terminalizes the collector's own id:

def leaf(self, params: tuple[ParamId, ...]) -> ItemNodeId:
    return ItemNodeId(path=self.path, names=self.names, params=params)

leaf(name, params) only works today because the flat layout has items contributing their own name segment. With a definition node the name gets added twice:

definition node      : test_x.py::test_a
definition.leaf(...) : test_x.py::test_a::test_a[1]   # current signature
definition.leaf(...) : test_x.py::test_a[1]           # params-only, == the flat form

so the flat item ids fall out for free and the explicit-nodeid plumbing goes away. Two production callers of leaf() on this branch (nodes.py:217, python.py:1716).

[nicoddemus]

@bluetech

However, I think the aim should be to reduce use of node IDs to a minimum in favor of using Node directly. For example we did it recently with the matchfactories change. That's better because the Node is the ultimate source of truth. That doesn't necessarily conflict with this work, but it may affect the consideration of the tradeoff between extra complexity and benefit.

I agree. However the concept of "node id" is used in a lot of places in the codebase and externally, so having a proper structure for it seems to be complementary.

BTW regarding this, if I search for "::" under src/ I see several cases of parsing remaining. It would be good to convert them in this PR so we can better evaluate the benefit.

Done.

Some cases didn't fit the original OpaqueNodeId, for example resolve_collection_argument wants the internal names up to the parametrization part ([). So I changed OpaqueNodeId from path/rest to path/names/params, with params being str|None with the contents of the inside of [] if any.

This made me realize that perhaps the introduction of ParamId was a bit hasty/overengineering. If we remove ParamId, then ItemNodeId can be simplified to path/names/params with params being str|None, removing the need for a separate OpaqueNodeId. This also simplifies things considerably given we don't need as_opaque() anymore. I made this change in a separate commit so we can evalute it.

Further: CollectionNodeId was introduced in order to avoid adding a runtime check in cases where we add a new child item to a leaf node. However having separate types makes code that needs to handle both more complex, so decided to also back that off, so ended up having a single NodeId class which parses the traditional string nodeid into a path/names/params structure, which is more inline with the original intent of the PR. Also in a separate commit.

---

@RonnyPfannschmidt

The underlying point: leaf creation should not add a name.

Done, and decided to rename leaf to with_params because I believe that's clearer.

[RonnyPfannschmidt]

the pedantic in me wants to see those 2 cases separated

[RonnyPfannschmidt]

for write_fspath_result - perhaps we want a non str nodid

[RonnyPfannschmidt]

im of the impression that the file collector id should be used there instead of a path

[RonnyPfannschmidt]

we should investigate directly using nodeids in some way here - but this is also a potential followup idea to limit scope between introducing the concept and permeating it trough all of the involved potential usage points

[RonnyPfannschmidt]

i still dislike not having a clear way to distinguish between know correctly made nodeids and opaque node ids

[RonnyPfannschmidt]

hmm, after reading how it was removed and the exact commit message of the commit, i think we should bring it back - its not surprising it was not used - its freshly introduced

[nicoddemus]

distinguish between know correctly made nodeids and opaque node ids

i think we should bring it back - its not surprising it was not used - its freshly introduced

Bring what back exactly? OpaqueNodeId?

As mentioned, the distinction only existed because we initially decided that the struct should contain a scope and explicit list of parameters, which I now think it was a bit over-engineering. If we treat a node id just as a path/names/params (and params being an opaque string), then every NodeId is valid, regardless if it was built "live" or from disk.