Replace string nodeids with a structured NodeId internally - #14758
Replace string nodeids with a structured NodeId internally#14758nicoddemus wants to merge 1 commit into
Conversation
877b612 to
957b2d3
Compare
0339c53 to
da6d73c
Compare
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
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
|
Should we expose the new node id types as public? I wonder if people will need at least for type annotations... |
33f8694 to
9ed3457
Compare
bluetech
left a comment
There was a problem hiding this comment.
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.
| def nodeid(self) -> str: | ||
| return str(self._id) | ||
|
|
||
| @nodeid.setter |
There was a problem hiding this comment.
Hmm is there code that mutates report.nodeid?
There was a problem hiding this comment.
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.
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.
@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. |
|
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 and then there's a number of other issues relating to that - so this really is about removing a mess and pain |
07039ee to
67c9d54
Compare
|
@RonnyPfannschmidt @bluetech @The-Compiler please take another look. 👍 |
bluetech
left a comment
There was a problem hiding this comment.
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.
|
Opus found a conflict between this and the 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 def leaf(self, params: tuple[ParamId, ...]) -> ItemNodeId:
return ItemNodeId(path=self.path, names=self.names, params=params)
so the flat item ids fall out for free and the explicit-nodeid plumbing goes away. Two production callers of |
67c9d54 to
599f551
Compare
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.
Done. Some cases didn't fit the original This made me realize that perhaps the introduction of Further:
Done, and decided to rename |
7b22e98 to
b0f4288
Compare
|
Ready for another review. |
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
i like where this is going
it will be on me to show the value of a more detailed parameter spec in the datastructure
i'll also work on providing a size/speed optimized variant of this soon (earlier experiments for this had unacceptable drawbacks)
| config=config, | ||
| session=self, | ||
| nodeid="", | ||
| nodeid=NodeId(path=""), |
There was a problem hiding this comment.
followup note - we might want to introduce some kind of root collection nodes that mist have a non empty path - the fake none/empty nodeid of session always feel like a wart to me thats hard to resolve
There was a problem hiding this comment.
Yeah it feels like a wart.
| return self | ||
|
|
||
| def __str__(self) -> str: | ||
| if self._str_cache is not None: |
There was a problem hiding this comment.
an idea for a potential followup - what if we used control characters for unit/record separators to annotate the parts of the string nodeid in a way thats not printed, but can be parsed easily and migrating to those
for now this is just a wild idea as i haven't investigated the downsides
There was a problem hiding this comment.
Special control characters? Not sure, what would this buy us? But yeah that might be an interesting follow up to explore.
There was a problem hiding this comment.
if we disallow those very characters to be unescaped, we get consistent parsing of "modern" nodeids
but potential downsides are a rabbithole im not ready for this year
|
I plan to merge this in the next few days, but let me know if you would like more time to re-review and I will hold off on the merge. |
|
It has my go ahead |
883c1e5 to
6645191
Compare
The changes have since been implemented
4aa6936 to
e15b997
Compare
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. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
e15b997 to
44a6553
Compare
|
Codecov is saying this has 99.4% diff coverage, but the page shows everything green:
Help? |
bluetech
left a comment
There was a problem hiding this comment.
Ran out of time to review everything, but some comments.
| base_path_part, *nodeid_part = nodeid.split("::") | ||
| # Only process path part | ||
| fullpath = self.rootpath / base_path_part | ||
| oid = NodeId.parse(nodeid) |
There was a problem hiding this comment.
For me oid is "object ID" (too much Postgres...). It's unclear to me what the "o" represents here. How about nodeid_? A bit ugly but what can you do..
| @@ -1345,12 +1342,12 @@ def notify_exception( | |||
| def cwd_relative_nodeid(self, nodeid: str) -> str: | |||
There was a problem hiding this comment.
In the interest of reducing stringy nodeid usage, I think it would be good to make this function accept and return NodeId. Would require also changing _locationline to take NodeId, my suggestion is to do the parse in pytest_runtest_logstart (topmost point where stringy nodeid is required).
|
|
||
|
|
||
| if sys.version_info >= (3, 12): # pragma: no cover | ||
| from typing import override as override |
| parts readily available, avoiding reparsing that information when needed and also making for | ||
| a better type than `str`. | ||
|
|
||
| The legacy ``::``-joined string form remains available (via ``str(node_id)``) |
There was a problem hiding this comment.
I wouldn't call the string form "legacy", it's how nodeids are provided by users and represented in output.
|
|
||
| @dataclasses.dataclass(frozen=True, slots=True, kw_only=True) | ||
| class NodeId: | ||
| """Structured address for a node in the collection tree. |
There was a problem hiding this comment.
| """Structured address for a node in the collection tree. | |
| """Structured identifier of a node in the collection tree. |
| assert "::()" not in nodeid | ||
| self._nodeid = nodeid | ||
| if not isinstance(nodeid, NodeId): # pragma: no cover | ||
| raise ValueError( |
There was a problem hiding this comment.
From what I can see there are a few plugins which do this unfortunately. I think it would be better to deprecate first if possible.
It would be good to mention name in the message, some plugins seem not to be aware of it, e.g.
There was a problem hiding this comment.
henry0312/pytest-pycodestyle#96 - aware - i can send them a fix
pytest-describe needs a path for the the new behaviour as well
lets deprecate for now and manage it here
i can phase it out later in my work for simplifying ctors
| raise TypeError("nodeid or parent must be provided") | ||
| self._nodeid = self.parent.nodeid + "::" + self.name | ||
| if isinstance(self, Item): | ||
| self._id = self.parent.id.child(self.name).with_params(None) |
There was a problem hiding this comment.
I think the with_params(None) here doesn't do anything, since child already returns None params.
| _id: NodeId | ||
|
|
||
| @property | ||
| def id(self) -> NodeId: | ||
| """The structured (non-string) form of ``nodeid``.""" | ||
| return self._id |
There was a problem hiding this comment.
It's inherited from Node now, so no need here, I think?
| nodeid = norm_sep(nodeid) | ||
| if path_str: | ||
| path_str = norm_sep(path_str) | ||
| if path_str is not None: |
There was a problem hiding this comment.
Didn't check the previous situation, but to make sure: the intention is that nodeid can remain None here in the else case?
| _id: NodeId | ||
|
|
||
| @property | ||
| def id(self) -> NodeId: | ||
| """The structured (non-string) form of ``nodeid``.""" | ||
| return self._id |

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.