Add SAMM - Semantic Aspect Meta Model exporter

Kostadin-Ivanov commented 2 months ago

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.

sschleemilch commented 1 month ago

The major refactoring got merged. Let me know if you need help adapting to it

SebastianSchildt commented 1 month ago

Meeting 08/27

needs reabse
rdflib already in pyproject.toml (generally all deps?)

Kostadin-Ivanov commented 1 month ago

I was on holidays, and now I am constantly checking for master updates and rebasing if there is any. Sorry for the delayed response on this.

I am now keep updating the code as per your feedback.

SebastianSchildt commented 1 month ago

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...

sschleemilch commented 1 month ago

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 pre-commit run --all-files

Kostadin-Ivanov commented 1 month ago

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 :)

Kostadin-Ivanov commented 1 month ago

Hello @sschleemilch and @SebastianSchildt , thank you very much for the hint on the pre-commit-set-up :)

This saved my life in vss-tools :D

There is still few minor TODOs, which I need to address on our my side, and then I hope that the code will be ready for final review.

Kostadin-Ivanov commented 1 month ago

Code is cleaned and ready for review.

erikbosch commented 1 month ago

MoM:

Please review (FYI @sschleemilch @erikbosch )
Could be merged if both approves
Stephen: Commit/PR does not include descriptions, no documentation

Kostadin-Ivanov commented 1 month ago

@erikbosch , I added a description of this PR and I can also add a README file with details and usage guide of this extension if required.

Kostadin-Ivanov commented 1 month ago

Hello @slawr ,

I am not sure where exactly shall I add the description of this PR. I tried to add a bit more information about it on top comment above.

There is a title / message for this PR: "Add SAMM - Semantic Aspect Meta Model exporter" but I am not sure if this would be sufficient.

With regards to User Documentation, I can add a README file if required.

slawr commented 1 month ago

Hello @slawr ,

I am not sure where exactly shall I add the description of this PR. I tried to add a bit more information about it on top comment above.

There is a title / message for this PR: "Add SAMM - Semantic Aspect Meta Model exporter" but I am not sure if this would be sufficient.

With regards to User Documentation, I can add a README file if required.

Hi Kostadin,

I don't have a defined hurdle in mind to clear (although thanks for asking) and you are right in stating the title should be a good hint to what it is about. I was mainly thinking of the engineer case where you have to understand what a commit or group of commits (PR) does. Perhaps some time later a maintainer needs to rebase some code or the community needs to understand the change. Then either a summary in the commit, or a useful description in the PR description is very helpful.

What is enough/appropriate is a grey area. Here I would say some kind of summary is better than none :) I think what you added to the PR helps a lot and would be the kind of thing I would have looked for. It tells me what the exporter outputs and provides some useful links.

User documentation is similarly undefined. Many utils do a very good job of providing runtime help that makes it self-defining. Although perhaps there is a setup burden for someone to get to that point. Personally I would say some README (or other form) that summarises the scope of the feature, any setup and how to run it would be a good addition.

erikbosch commented 1 month ago

Hi @Kostadin-Ivanov

Concerning documentation - a possible solution is:

Update the printout at https://github.com/COVESA/vss-tools/blob/master/docs/vspec.md#vspec-exporters listing all available exporters
Create file samm.md similar to https://github.com/COVESA/vss-tools/blob/master/docs/apigear.md where you give some general info/links on SAMM, design decisions/limitations and one or more usage examples on how you call "vspec export samm ...". I would appreciate a few lines on how you validate the output from the vspec samm exporter. Like if there is a samm compiler/validator you can install and use.
In https://github.com/COVESA/vss-tools/blob/master/docs/vspec.md#exporter-docs add a link to samm.md

Kostadin-Ivanov commented 1 month ago

Thank you very much for the hints, Erik (@erikbosch ) and Stephen (@slawr).

I will action these later today and will let you know once I am done with the docs.

erikbosch commented 1 month ago

It would be great to have one or more test cases that verifies that the new exporter is still functional. That would help to detect regressions if we change internals in tooling.

Two possible approaches

Extend generic tests

We have a framework where we run tests and compare with previous result to detect regressions. See https://github.com/COVESA/vss-tools/blob/master/tests/vspec/test_generic.py#L56

If you would like to use for this SAMM you need to first extend the code snippet above, but then also add expected.samm files in all cases where the generic framework is used (look for expected.csv). The workflow here is to run that testcase vspec command manually and store output as expected.samm and then verify that pytest works

Single test

You could also add a specific test in for example tests/samm where you run the tool on an example vspec and verifies that the result works as expected.

Kostadin-Ivanov commented 1 month ago

It would be great to have one or more test cases that verifies that the new exporter is still functional. That would help to detect regressions if we change internals in tooling.

Two possible approaches

Extend generic tests

We have a framework where we run tests and compare with previous result to detect regressions. See https://github.com/COVESA/vss-tools/blob/master/tests/vspec/test_generic.py#L56

If you would like to use for this SAMM you need to first extend the code snippet above, but then also add expected.samm files in all cases where the generic framework is used (look for expected.csv). The workflow here is to run that testcase vspec command manually and store output as expected.samm and then verify that pytest works

Single test

You could also add a specific test in for example tests/samm where you run the tool on an example vspec and verifies that the result works as expected.

Hello @erikbosch, I am not sure how long it might take to prepare such tests. Could we do it as a post-release update / improvement?

So far, I provided a samm.md with more information about this exporter, some user guide and examples how to validate generated aspect models.

With regards to validation - briefly, this can be done using the ESMF CLI tool with a simple command like:

samm aspect vss_ttls/com.covesa.vss.spec/5.0.0/Vehicle.ttl validate

The ESMF - Aspect Model Editor (AME) can also be used for manual validation of generated aspect models.

sschleemilch commented 1 month ago

I am sorry for the opinionated Feedback 😄

erikbosch commented 1 month ago

Hello @erikbosch, I am not sure how long it might take to prepare such tests. Could we do it as a post-release update / improvement?

So far, I provided a samm.md with more information about this exporter, some user guide and examples how to validate generated aspect models.

With regards to validation - briefly, this can be done using the ESMF CLI tool with a simple command like:

samm aspect vss_ttls/com.covesa.vss.spec/5.0.0/Vehicle.ttl validate

The ESMF - Aspect Model Editor (AME) can also be used for manual validation of generated aspect models.

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.

erikbosch commented 1 month ago

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.

erik@debian6:~/vss-tools/tests/vspec/test_allowed$ vspec export samm -u ../test_units.yaml -q ../test_quantities.yaml --vspec test.vspec --target-folder xxx
[13:09:37] INFO     Loading VSS Tree...                                                                                                                                                                       vss2samm.py:182

           INFO     Loaded 'VSSQuantity', file=/home/erik/vss-tools/tests/vspec/test_allowed/../test_quantities.yaml, elements=6                                                                       units_quantities.py:34
           INFO     Loaded 'VSSUnit', file=/home/erik/vss-tools/tests/vspec/test_allowed/../test_units.yaml, elements=7                                                                                units_quantities.py:34
           INFO     VSpecs loaded, amount=1                                                                                                                                                                      vspec.py:122
[13:09:38] INFO     No signals selected.                                                                                                                                                                      vss2samm.py:212
                    Creating model for the whole tree.                                                                                                                                                                       

           INFO     Update output: 'xxx' with ESMF namespace: 'com.covesa.vss.spec' and VSS Version: '1.0.0'.                                                                                                 vss2samm.py:214

           INFO     Generating SAMM output...                                                                                                                                                                 vss2samm.py:224

           ERROR                                                                                                                                                                                              vss2samm.py:280
                    There was a problem with VSS to ESMF processing.                                                                                                                                                         
           ERROR                                                                                                                                                                                              vss2samm.py:281
                    Error: unsupported operand type(s) for +: 'int' and 'str'                                                                                                                                                
erik@debian6:~/vss-tools/tests/vspec/test_allowed$

Kostadin-Ivanov commented 1 month ago

I am sorry for the opinionated Feedback 😄

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 :)

Kostadin-Ivanov commented 1 month ago

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.

erik@debian6:~/vss-tools/tests/vspec/test_allowed$ vspec export samm -u ../test_units.yaml -q ../test_quantities.yaml --vspec test.vspec --target-folder xxx
[13:09:37] INFO     Loading VSS Tree...                                                                                                                                                                       vss2samm.py:182

           INFO     Loaded 'VSSQuantity', file=/home/erik/vss-tools/tests/vspec/test_allowed/../test_quantities.yaml, elements=6                                                                       units_quantities.py:34
           INFO     Loaded 'VSSUnit', file=/home/erik/vss-tools/tests/vspec/test_allowed/../test_units.yaml, elements=7                                                                                units_quantities.py:34
           INFO     VSpecs loaded, amount=1                                                                                                                                                                      vspec.py:122
[13:09:38] INFO     No signals selected.                                                                                                                                                                      vss2samm.py:212
                    Creating model for the whole tree.                                                                                                                                                                       

           INFO     Update output: 'xxx' with ESMF namespace: 'com.covesa.vss.spec' and VSS Version: '1.0.0'.                                                                                                 vss2samm.py:214

           INFO     Generating SAMM output...                                                                                                                                                                 vss2samm.py:224

           ERROR                                                                                                                                                                                              vss2samm.py:280
                    There was a problem with VSS to ESMF processing.                                                                                                                                                         
           ERROR                                                                                                                                                                                              vss2samm.py:281
                    Error: unsupported operand type(s) for +: 'int' and 'str'                                                                                                                                                
erik@debian6:~/vss-tools/tests/vspec/test_allowed$

@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.

Kostadin-Ivanov commented 1 month ago

A lot of code to be reviewed (+2k lines). Not finished yet but first feedback provided 👯

We've been working on this script as "external" to vss-tools tool for almost year and a half :)

sschleemilch commented 1 month ago

I removed the try/catch blocks so we should see the whole error + stacktrace.

Now you know what I meant hehe

Kostadin-Ivanov commented 1 month ago

Hello Sebastian (@sschleemilch) and Erik (@erikbosch), could you please have a lok at my responses to your feedback and "resolve" them if your are happy with my updates or add your additional thoughts if I've missed something.

Also, could you tell me what will be next step, if you are happy with the updates. You will merge this PR to main branch or I should do it?

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 :)

erikbosch commented 1 month ago

Hello Sebastian (@sschleemilch) and Erik (@erikbosch), could you please have a lok at my responses to your feedback and "resolve" them if your are happy with my updates or add your additional thoughts if I've missed something.

Also, could you tell me what will be next step, if you are happy with the updates. You will merge this PR to main branch or I should do it?

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 :)

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

Kostadin-Ivanov commented 1 month ago

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 :)

COVESA / vss-tools

Add SAMM - Semantic Aspect Meta Model exporter #394