Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aasx.adapter: fix semantic id type deserialization of ModelReference #337

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JGrothoff
Copy link
Contributor

Many AASX packages include semantic ids with external or model references.

Concept descriptions are lost when reading and writing back to AASX

Problem

When a semantic id with a ModelReference is deserialized its type is currently set to the abstract type Referable.
When the aasx is serialized again and the semantic id refers to a concept description, it is skipped, because only concrete ModelReference types are looked up and serialized.

Solutions

  • Suggested: Set the type of the semantic id to the concrete type determined by the last key element.
  • Alternative: Skip the type check as errors are caught anyway (EAFP style). Maybe lowering the loglevel to warning or info.

Writing aasx with external references as semantic ids produce many info logs

Problem

Often an external reference with a global id (e.g. to ECLASS IRDIs) is used as semantic id.
A typical technical data submodel may easily contain around 100 of them.
Writing to an aasx will print an info message for each semantic id, that is not a model reference to an concept description, which clutters the output.

Solutions

  • Suggested: skip external references without log message
  • Alternative: lower logging level to debug or delete log info.

@JGrothoff JGrothoff marked this pull request as draft November 19, 2024 08:47
@JGrothoff JGrothoff force-pushed the fix/write-aasx-cd-external branch from 968597b to 9f0dea6 Compare November 19, 2024 08:49
logger.warning("type %s of last key of reference to %s does not match reference type %s",
keys[-1].type.name, " / ".join(str(k) for k in keys), type_.__name__)
return object_class(tuple(keys), type_, cls._construct_reference(_get_ts(dct, 'referredSemanticId', dict))
return object_class(tuple(keys), last_key_type,
cls._construct_reference(_get_ts(dct, 'referredSemanticId', dict))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this won't introduce side effects, as the _construct_model_reference method is used frequently.
We could addittionaly add a check and fallback to type_ if last_key_type is type(None).

@s-heppner
Copy link
Contributor

Thank you very much for your input!
This is indeed a quite complicated problem, as the specification was not clear about which ReferenceType to use and therefore we have lots of weird AASX files in the wild.

I agree that our SDK should not discard content without warning when reading and writing again, so that is definitly something that needs to be addressed.

Regarding your suggestions: I need to do some more thinking about which solution is the one with the least repercussions. I'll bring this discussion to our dev meeting and will get back to you.

@JGrothoff
Copy link
Contributor Author

Thanks for investigating the topic so far!
I will try to add or update the tests to cover the issue (thats why i marked this as Draft).

@JGrothoff JGrothoff force-pushed the fix/write-aasx-cd-external branch 2 times, most recently from 904e747 to 381d6a9 Compare November 21, 2024 16:36
@JGrothoff JGrothoff marked this pull request as ready for review November 21, 2024 16:38
@JGrothoff
Copy link
Contributor Author

I added a test that covers all semantic ids that are expected to be ModelReferences for the same type.

@s-heppner
Copy link
Contributor

This is what we concluded in our meeting:

  • We cannot allow writing wrong AASXs. If the ConceptDescription of the ModelReference is not found, we should raise an Error as the AASX is wrong.
  • However, we should obviously not silently discard content, as in not warning the user about the issue.
  • We should try to declutter our output (if there's not actually an issue with the ExternalReference). This message may be now unnecessary and might have been historically grown

Sadly, I did not have time yet to look deeper into the matter.

@JGrothoff JGrothoff force-pushed the fix/write-aasx-cd-external branch from 766c5d4 to ee006dc Compare December 9, 2024 13:35
@JGrothoff JGrothoff force-pushed the fix/write-aasx-cd-external branch from ee006dc to a441d20 Compare December 9, 2024 13:41
@JGrothoff
Copy link
Contributor Author

  • We cannot allow writing wrong AASXs. If the ConceptDescription of the ModelReference is not found, we should raise an Error as the AASX is wrong.

I rebased on main and changed to a re-raise for those cases. It will raise an error if the reference is a ModelReference and has the type ConceptDescription, but it can't be resolved in the given object_store.
Of couse, we could change the Exeception class here, but i will leave that up to you.

This will still not check if

  • an abstract Reference or
  • a ModelReference with type other than a ConceptDescription
    can be resolved and is contained in the object_store (or in the objects_to_be_written).
    But it will log an info message about that, like before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants