-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add SAMM - Semantic Aspect Meta Model exporter #394
Conversation
ad4856c
to
5ebbf86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of code to be reviewed (+2k lines). Not finished yet but first feedback provided 👯
|
||
log.debug("Writing graph to \n -- file: '%s' \n -- location: '%s'", file_name, path_to_file) | ||
|
||
log.debug("Current working directory: '%s'", os.getcwd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vss_tools tries to use the more modern Pathlib instead of os.*
methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case: Path.cwd()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the Pathlib to create folders and files dynamically, because I use it further in this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
minor = 0 | ||
patch = 0 | ||
|
||
if vss_tree and hasattr(vss_tree, "children") and len(vss_tree.children) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is save to call just if vss_tree.children
for an Anytree node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
# Exporter specific options | ||
|
||
@clo.option( | ||
'-sigf', '--signals-file', 'signals_file', show_default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until now we followed the posix strategy of single dash options being single characters. Not sure if we should change that here. The signals_file
parameter is not needed. show_default
is not needed if there is no default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the input is a file, check how to define it propery with cli_options
as a reference using Pathlib.
It has the benefit that existence and permissions can be checked on cli level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
) | ||
@clo.option( | ||
"--split-depth", "-spld", "split_depth", | ||
required=False, default=1, show_default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required=False
is the default, so not needed. split_depth
not needed. type
should be set to int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
) | ||
@clo.option( | ||
"--split", "-spl", "split", | ||
required=False, is_flag=True, default=False, show_default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For flags it can be useful to define them with option("--split/--no-split", default=False)
. It has is_flag
implicitly and provides two quite concise options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return f'{self.ns}{self.value}' | ||
|
||
|
||
class SammCConcepts (Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty similar if not the same as SammConcepts
Maybe define a common parent class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SammConcepts and SammCConcepts use different uri's (namespaces):
SammConcepts has: self.ns = f"{samm_base_namespace}:meta-model:{cnfg.SAMM_VERSION}#"
where
SammCConcepts has: self.ns = f"{samm_base_namespace}:characteristic:{cnfg.SAMM_VERSION}#"
I will see how can I combine these and override just their initialization part with these namespaces.
Thanks for the catch :)
# which then can be further manipulated. | ||
# | ||
# For example: filter the children of cloned VSSNode and add them to its cloned (copied) version. | ||
# REASON FOR THIS is that: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the case when e.g. using copy.deepcopy(). However, the performance of deepcopy is not great since it will essentially copy the complete tree including all parents and therefore all children etc. This is one of the reasons I implemented an own copy
method: https://github.com/COVESA/vss-tools/pull/382/files#diff-156f359c987ddf2af79c8b9bf50be3c1899d7a3e389865b0fe7b89812d62bc51R69.
Maybe we will merge the refactoring this week. That will simplify a lot of things but will cause conflicts with this one unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of this.
Reason for this copy was explained in the comment above this: clone_vss_node function.
This was needed when this exporter was created as external tool which was using provided by vss-tools functionality so I am open for improvements if there is a way to reduce this effort for filtering of selected signals / branches etc.
with regards to possible conflicts, no worries, I will deal with them :)
# which was used for the creation of vss_node, | ||
# i.e. its corresponding name in the used for creation of VSSNode, dictionary. | ||
properties_to_clone = { | ||
"$file_name$" : "$file_name$", # noqa: E203 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be simpler to specify things not to clone? But yeah, see comment before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my previous comment I was trying to keep only the fields which my script was going to use, instead of copying whole VssNode, but again as I said I am OK for further refactoring of this and if possible to build "filtered" tree in different way I can even remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "cloning" and related logic is now removed.
Done
or (vss_node.is_leaf | ||
and hasattr(vss_node, 'datatype') | ||
and vss_node.datatype is VSSDataType.BOOLEAN | ||
and (vss_node.name.startswith('Is') or vss_node.name.startswith('is')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vss_node.name.lower().startswith("is")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
# TODO: Currently this is a workaround to read the Vehicle.VersionVSS, which is provided from COVESA/VSS | ||
# and provides the supported VSS version. | ||
# In best case this functionality should be provided by the vspec tool, loaded in this script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean by a method in the common part of vss_tools
right? Something like:
def get_vss_version(root: VSSNode) -> tuple[int]:
...
return major, minor, patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
As per my previous comments, this converter was built as external to vss-tools script and I could not find way to get version of current VSS tree like: vss_node.get_vss_version(), which then will return either string in the form: major.minor.patch or a tuple as you suggest.
056488f
to
85e7f70
Compare
The major refactoring got merged. Let me know if you need help adapting to it |
85e7f70
to
9dba80e
Compare
Meeting 08/27
|
b513803
to
1c3d33d
Compare
I was on holidays, and now I am constantly checking for master updates and rebasing if there is any. I am now keep updating the code as per your feedback. |
d0b68fa
to
be3bdf8
Compare
You seem to be mostly dying by our linter enforcing stricter code quality checks. Note the section about pre-commit install in the main Readme: https://github.com/COVESA/vss-tools?tab=readme-ov-file#pre-commit-set-up I found the easiest way is, installing pre-commit, "touching" the file CI complains about by adding a space somewhere (i.e. so you do have something to add/commit), do the commit, and likely the check will fail AND it will have repaired the file for you. Check whether changes are ok, add the file again, and commit... |
Or run |
428119c
to
1a2a13e
Compare
Thank you both :) I was trying to reuse some of these pre-checks locally, but for some reason they were passing OK on my side and then failing here. Most likely, I was missing some pre-check locally on one side and on the other, I wanted to do it in smaller updates, just not to mess something else. Fingers crossed that I will manage to clear all of these style checks soon :) |
# NOTE: this is a special case for the OBD branch, which children are marked as deprecated from VSS 5.0, | ||
# but the branch itself is not marked as deprecated. | ||
deprecated_children = [] | ||
if hasattr(vss_node.data, "type") and vss_node.data.type is NodeType.BRANCH: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every vssnode has type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Skip parsing of VSSNode is branches, which have only deprecated children | ||
# NOTE: this is a special case for the OBD branch, which children are marked as deprecated from VSS 5.0, | ||
# but the branch itself is not marked as deprecated. | ||
deprecated_children = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the whole part a single liner?
deprecated_children = list(filter(lambda n: n.data.deprecation, vss_node.children))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint.
I updated the code as advised.
node_char_uri = ttl_builder.add_node_branch_characteristic(graph, vss_node, node_uri) | ||
|
||
if ( | ||
hasattr(vss_node.data, "instances") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is a branch it will always have instances filled therefore if vss_node.data.instances
should suffice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting similar to previous screenshot VSSRaw error if I've removed the hasattr check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I updated the checks as advised.
graph.add((node_char_uri, SammConcepts.DATA_TYPE.uri, node_entity_uri)) | ||
|
||
# Populate Entity properties if the current vss_node holds any child node | ||
if hasattr(vss_node, "children") and len(vss_node.children) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just if vss_node.children
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just start the for loop. It will be a noop if there are no children
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Removed this redundant check for children length.
ttl_helper = None | ||
|
||
|
||
def __setup_environment(output_namespace, vspec_version, split_depth: int) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that setup function looks quite suspicious. Why do we need dynamic imports here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal here was to get user input, which then can be used to initialize the script config, and later to load the: samm_concepts, vss_helper and ttl_helper which are working with some of the config fields, which depend on the user input at the time of call of the script.
Again, this was just my understanding of handling this case, but if there is a better way to do it, I will be happy to do some refactoring :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use function/class arguments does not work? If not, you could have a global state such as config.py
that contains objects that get filled at the script start and other modules can then import and use.
EDIT: I just noticed that you have a config.py
already. Then I do not get why we need dynamic imports... They can just import config and use those variables, can't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a call to init the config with user input data:
def __setup_environment(output_namespace, vspec_version, split_depth: int) -> None: # Initialize config before to load other helpers / libraries, based on defined by user input, config data cfg.init(output_namespace, vspec_version, split_depth)
without this, I was getting errors for undefined variables in the dynamically loaded _helper(s)
Sorry for the "code" but for some reason I could not upload a screenshot.
|
||
if vss_tree.children and len(vss_tree.children) > 0: | ||
# Get the VersionVSS node so to extract current VSS version from it. | ||
vss_version_node = next(filter(lambda vs_child: vs_child.name == "VersionVSS", vss_tree.children), None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also a tree method which might be what you want:
vss_tree.get_child(vss_tree.get_fqn() + ".VersionVSS")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I am sorry for the opinionated Feedback 😄 |
c5cccbf
to
1df07d4
Compare
Yes, it could be done in a separate PR as I see it. The important part as I see it is that we long term get some test cases so we detect regressions (if any) when introduced. |
@@ -0,0 +1,220 @@ | |||
# Vspec - Semantic Aspect Meta Model (SAMM) exporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition - now you have a better documentation that most other tools :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hint on this :)
Tested the tool on one of the test vspecs we have. It gives an error because of some exception, most likely some syntax for allowed which is not supported yet. As this construct is not (yet) used in std catalog I do not see it as critical, even if error message possibly could be improved.
|
No worries, there is nothing to apologize for and thank you for the good feedback and hints :) I am currently implementing all of your latest feedback and hope to be ready for final review by end of today :) |
1df07d4
to
b185813
Compare
@erikbosch - could you try to run it again. I removed the try/catch blocks so we should see the whole error + stacktrace. I'd say that there is some problem with the test.vspec but I don't know its contents. |
d0802d5
to
d5af4ec
Compare
We've been working on this script as "external" to vss-tools tool for almost year and a half :) |
d5af4ec
to
0e84663
Compare
Now you know what I meant hehe |
# | ||
|
||
|
||
def __add_node_tuple(graph: Graph, subject_uri: URIRef, predicate_uri: URIRef, object_data: Literal | URIRef) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might this be a candidate to be an own exporter? Like vspec export ttl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I remember, there used to be a ttl exporter in the contrib folder, but I think that this is obsolete and it was generating different formatted ttls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be reasonable to separate it into an exporter that exports it correctly?
Maybe we can reduce the complexity of that exporter then a bit? Does not need to happen now of course...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said, there is already this vss2ttl which seems to do this, but when I look at its code, it seems to be a different type of ttl format.
Both helpers: ttl_helper and ttl_builder_helper are quite bound to the SAMM formatted ttls. for this reason I am not 100% how easy would be to make these more generic so they can be used for more general purpose and why not as a stand alone ttl exporter.
Maybe as another improvement, we can think about such refactoring as further improvement.
It would be interesting if then we can reuse functionality of one exporter by another exporter, but we will see if we go in this direction.
By the way, just to add, I reused the try/catch, which is no longer present in the vss2samm, from there :)
fa3c890
to
90c4b76
Compare
Signed-off-by: Kostadin Ivanov (BD/TBC-BG) <kostadin.ivanov@bosch.com>
90c4b76
to
dc1cfbf
Compare
Hello Sebastian (@sschleemilch) and Erik (@erikbosch), Also, could you tell me what will be next step, if you are happy with the updates. And one more thing, after we merge this PR, when it will be released in the master VSS-TOOLS and would it be updated in the COVESA VSS as well? Thank you :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider this one good to go now. later changes, if any, can be performed by subsequent PRs.
I think this one is good to go now. Only maintainers have write rights and can merge, so I will click the merge button. Then it will show up on master branch. We are planning a 5.0 release soon, but have not yet agreed on exactly when. If needed we could update the submodule reference from vss to include this one, and then possibly integrate into the CI and Makefile for vss as well, to make sure that it still pass. After merge it would be good if you could add at least a single test case that verifies that the tool runs and produces expected output. That would help us detect future regressions, if any |
Thank you very much @erikbosch . I will take note on the further updates / improvements / tasks, that we identified during this PR discussion and make sure that we follow on them. With regards to the tests, I will have a look at how other tests are organized and try to add some. Thanks again to everybody for your support :) |
This is new functionality to allow for conversion (export) of VSS specification into .ttl formatted Semantic Aspect Meta Models, which can be further used in the ESMF tools like Aspect Model Editor and SAMM CLI - for further application integrations.