Convert OdxLinkId.doc_fragments and OdxLinkRef.ref_docs to tuple #410

zariiii9003 · 2025-04-04T06:40:33Z

As suggested in #405.

I tried converting OdxLinkRef into a frozen dataclass, but subclasses assign attributes after instantiation.

zariiii9003 · 2025-04-04T06:54:32Z

@andlaus Unrelated: i think there are a lot of mypy issues hidden by the Noreturn in the odxraise method. It should be NoReturn | None and the TYPE_CHECKING should be removed.

andlaus · 2025-04-04T09:23:43Z

The Noreturn is on purpose: Semantically odxraise() should be considered identical to raise. The difference is that in non-strict mode it becomes a no-op, i.e. the reason why there is often code after the odxraise() call is to provide a sensible fallback for non-strict mode (which does not make any guarantees about undefined behavior or working in general, just that it "does its best"). So odxraise() should stay Noreturn in the main branch, but for development purposes, it I agree that could be temporarily changed to None to uncover the issues which you mentioned (I suppose that you will get a lot of mypy errors then...)

andlaus · 2025-04-04T09:28:30Z

I tried converting OdxLinkRef into a frozen dataclass, but subclasses assign attributes after instantiation.

IIRC, that was also the issue I stumbled over, ParentRef was particularly bad because it does quite a bit of reference resolution...

zariiii9003 · 2025-04-04T17:48:57Z

A few more changes after a bit of research. To my understanding there are two options: the doc fragments are either just the category, or the category+diag_layer.

i changed the type annotation of OdxDocContext.doc_fragments to reflect that. The category context is now completely defined in Database._process_xml_tree(). The DiagLayerContainer then creates a new context for its layers.

odxtools/comparamsubset.py

odxtools/diaglayercontainer.py

odxtools/diaglayers/diaglayerraw.py

odxtools/odxdoccontext.py

andlaus

looks pretty good. when the match statement is replaced and it is verified that ODX 2.0 does not regress (you might need to help out on that front @kayoub5) I think this should be merged...

odxtools/database.py

tests/test_variant_patterns.py

kayoub5 · 2025-04-05T12:26:16Z

@nada-ben-ali could you check that odx 2.0 compatibility is not broken with those changes ?

kayoub5 · 2025-04-05T16:17:46Z

odxtools/diaglayers/hierarchyelement.py

@@ -31,7 +31,7 @@
 from .hierarchyelementraw import HierarchyElementRaw

 if TYPE_CHECKING:
-    from .database import Database
+    from ..database import Database


mypy didn't catch this ?

@zariiii9003: how did you find this? if using tools, can/should we change the CI system to do the same?

I noticed red squiggly lines in my IDE. I also wonder why both ruff and mypy seem to ignore it

ok. Seems like you should continue to let your IDE look at odxtools. mine (emacs) is not sophisticated enough ;)

andlaus · 2025-04-07T12:24:40Z

sorry for the conflict due to #411. as I see it, the .doc_frags attribute should not be defaulted anyway...

andlaus · 2025-04-07T18:11:37Z

thanks for rebasing. as soon as @nada-ben-ali reports back, I'll merge this.

I don't know why this blew up my smoke testing schript only now... Signed-off-by: Andreas Lauser <andreas.lauser@mercedes-benz.com> Signed-off-by: Alexander Walz <alexander.walz@mercedes-benz.com>

andlaus · 2025-04-10T10:24:06Z

For some strange reason, I ran into some DOCREF-related issue with this. I don't know why it only crept up with this PR, but I made a "meta pull request" for your PR here: zariiii9003#1 . would be great if you merged it...

fix DOCREF for PARENT-REFs using implicit document fragments

andlaus · 2025-04-10T13:00:21Z

@nada-ben-ali: were you able to test this with ODX 2.0? I think it is relatively unlikely that it regresses something on that front, and since I get a weird performance regression which goes away with this PR, I'd like to get it merged quickly...

andlaus · 2025-04-14T08:55:04Z

@nada-ben-ali, @kayoub5: If I do not get any objections to this PR, I'll merge it on Thursday...

nada-ben-ali · 2025-04-16T07:06:12Z

@zariiii9003 @kayoub5 @andlaus sorryy I've just seen the email notifications. I'll begin checking now.

nada-ben-ali · 2025-04-16T08:30:18Z

I got the following exception while loading the pickle file for some pdx files we have:

I'm checking this...

nada-ben-ali · 2025-04-16T15:30:17Z

I got the following exception while loading the pickle file for some pdx files we have:

I'm checking this...

the issue is caused by #407.
@kayoub5 @andlaus after removing the changes introduced in #407 and #411 (which have not been included in a release yet), no issues are observed with the current merge request changes.

I'm currently checking the impact of the dataclass as Kw_only on the unpickling operation and how we can fix it. It only occurs with some files, not all of them. For now, I have only identified the MRs that caused the issue.

andlaus · 2025-04-16T16:36:58Z

ok, does this mean that this PR can be merged and I should only prepare the next release once the unpickling issues are fixed? are you on it? (I do not currently use pickling, so I have a hard time reproducing it.)

nada-ben-ali · 2025-04-17T07:30:53Z

ok, does this mean that this PR can be merged and I should only prepare the next release once the unpickling issues are fixed? are you on it? (I do not currently use pickling, so I have a hard time reproducing it.)

@andlaus yes, we can merge this, and I'll work on fixing the unpickling problem.

andlaus · 2025-04-17T07:57:30Z

ok, cool. merging; thanks everyone!