🎨 Fixed some samm exporter findings #402

sschleemilch · 2024-09-10T11:51:16Z

Fixed the need of dynamic imports.
@Kostadin-Ivanov Please confirm it still works. Tests are fine though

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>

erikbosch · 2024-09-10T14:29:11Z

MoM:

OK to merge when verified by Erik, Kostadin. S Schleemilch

Kostadin-Ivanov · 2024-09-10T16:32:22Z

This check was used especially for case, when user provides list of selected signals and the VSS tree - instances are expanded.

Before the refactoring, there was a flag: expanded, which was stating whether the corresponding VSSNode is expanded or not, but after the refactoring, this field was somehow lost.

In my code, there is two functions: is_vss_node_selected_for_processing and filter_vss_tree which are used to filter out just selected VSSNodes, in case when User provides a list with selected signals.

The idea is that, lets say if there is a selected signal for: Vehicle.Cabin.Door.Window.Position, then the result will just have that particular signal in all corresponding Door instances i.e., if lets say the Door Window is not selected, but its signal: Position is selected, then the Door.Window.Position tree should be present in the filtered VSS tree and the other Door and Window signals should not be present in the "filtered" VSS Tree.

I just tested your code, and it does not seem to provide the expected result.
I had some selected signals and I still got the aspect model of the whole Door tree i.e. as if I have not selected any signals.

Tomorrow I will try to get back to you with better explanation of the logic behind this functionality.

For the moment, this update does not seem to provide required output when I run the code with some selected signals and using the default: expand nodes mode.

Also, there seems to be some problem when I use the --no-expand although it was working fine last week i.e., when this option was used, the Door had proper Row1/2, Driver/Passenger side etc. branches but now these are missing.

I will have to further investigate and fix this on our side.

sschleemilch · 2024-09-10T19:55:52Z

Hello @sschleemilch ,

This check was used especially for case, when user provides list of selected signals and the VSS tree - instances are expanded.

Before the refactoring, there was a flag: expanded, which was stating whether the corresponding VSSNode is expanded or not, but after the refactoring, this field was somehow lost.

In my code, there is two functions: is_vss_node_selected_for_processing and filter_vss_tree which are used to filter out just selected VSSNodes, in case when User provides a list with selected signals.

The idea is that, lets say if there is a selected signal for: Vehicle.Cabin.Door.Window.Position, then the result will just have that particular signal in all corresponding Door instances i.e., if lets say the Door Window is not selected, but its signal: Position is selected, then the Door.Window.Position tree should be present in the filtered VSS tree and the other Door and Window signals should not be present in the "filtered" VSS Tree.

I just tested your code, and it does not seem to provide the expected result. I had some selected signals and I still got the aspect model of the whole Door tree i.e. as if I have not selected any signals.

Tomorrow I will try to get back to you with better explanation of the logic behind this functionality.

For the moment, this update does not seem to provide required output when I run the code with some selected signals and using the default: expand nodes mode.

Also, there seems to be some problem when I use the --no-expand although it was working fine last week i.e., when this option was used, the Door had proper Row1/2, Driver/Passenger side etc. branches but now these are missing.

I will have to further investigate and fix this on our side.

Okay now I am a bit confused especially about the --no-expand part that does not work.

Current master (b898562)

Testfile:

Vehicle:
  type: branch
  description: Vehicle

Vehicle.Door:
  type: branch
  description: Door
  instances:
  - Row[1,2]
  - DriverSide
  - PassengerSide

--no-expand

Vehicle
instances=[]
└── Door
    instances=['Row[1,2]', 'DriverSide', 'PassengerSide']

--expand

Vehicle
instances=[]
└── Door
    instances=[]
    ├── Row1
    │   instances=[]
    │   ├── DriverSide
    │   │   instances=[]
    │   └── PassengerSide
    │       instances=[]
    └── Row2
        instances=[]
        ├── DriverSide
        │   instances=[]
        └── PassengerSide
            instances=[]

So not sure whether you meant the samm exporter or in general.

I also did not quite get the part about filtering. Normally the method for users to not use some branches is to define an overlay file and add delete: true to the parts that should not be included.

To stick with the test example before: To delete complete Row2 and therefore filtering it we can define an overlay file like that:

# overlay.vspec
Vehicle.Door.Row2:
  delete: true

Now we can add that overlay as an argument -l overlay.vspec and will then get a tree like this:

Vehicle
└── Door
    └── Row1
        ├── DriverSide
        └── PassengerSide

If you are trying to achieve the same but from a different user input than the cli there is a delete_nodes() that can delete given nodes of a tree. In our main.py we use it like that:

tree.delete_nodes(findall(tree, filter_=lambda n: n.get_vss_data().delete))

Kostadin-Ivanov · 2024-09-11T07:33:58Z

Hello @sschleemilch .

when we started work on the vss2samm exporter, there was an option to provide a list of selected signals, as explained in the vss2samm docs --signals-file or -sigf option.

Basically, it is used to define a list of signals that you want to have, and everything else is discarded from the tree.

I'd say that its result will be same as you explain with overlay deletion, but a bit shorter way i.e., instead of declaring each signal / branch to be "deleted", the user just specifies the list of signals he/she wants to have in the tree.

The problem with not proper handling of --no-expanded VSS Tree is related to the vss2samm exporter. Honestly, I am not sure how this "slipped", because I was testing it multiple times last week and it wa sworking fine in both cases: --expand and --no-expand, with and without selected signals.

Anyway, I will investigate this and will provide a fix for it, later today.

Now, with regards to this PR.
As I mentioned, for some reason the logic of this is_node_expanded somehow stopped to work as expected.
I will refactor it so it will be stable and returns correct as expected.

sschleemilch · 2024-09-11T08:24:48Z

Now, with regards to this PR.

Note that I just mentioned the diff I would like to apply but I did not apply it here. So the current diff does not change the handling of the instance thingy

I'd say that its result will be same as you explain with overlay deletion, but a bit shorter way i.e., instead of declaring each signal / branch to be "deleted", the user just specifies the list of signals he/she wants to have in the tree.

I understand. However, isn't that then also easy to do with things available?
Just iterate over the tree and set delete: true to all nodes that are not in your list.
Afterwards you can do:

tree.delete_nodes(findall(tree, filter_=lambda n: n.get_vss_data().delete))

I am still not sure why you need to know about the expanded state here.

Oh and if you think that it does not make sense that people are using --no-expand with the samm exporter: Not every exporter needs to support that option. You can then just get rid of the option and not pass it to get_trees or pass true to the expansion parameter.

sschleemilch · 2024-09-11T08:26:15Z

We could think about providing a core tree method to filter for explicit signals aswell

Kostadin-Ivanov · 2024-09-11T08:59:49Z

Oh and if you think that it does not make sense that people are using --no-expand with the samm exporter: Not every exporter needs to support that option. You can then just get rid of the option and not pass it to get_trees or pass true to the expansion parameter.

Makes sense.
In fact, before we go for this contribution, we had the default: --no-expand, i.e. our script was loading the VSS tree always --no-expand-ed.

However, with this contribution, we wanted to preserve general vss-tools (vspec) behavior, and allow the option that user can go with expanded i.e., instantiated tree, and not expanded one, i.e. the one with single instances etc.

Now, the difference of expanded and not expanded tree, when creating aspect models is that:

--expand will result in quite a lot of duplicate nodes. If we stick with the Door example, then this will result in:
Vehicle.CabinDoor.Row1.DriverSide.IsLocked
Vehicle.CabinDoor.Row1.PassengerSide.IsLocked
Vehicle.CabinDoor.Row2.DriverSide.IsLocked
Vehicle.CabinDoor.Row2.PassengerSide.IsLocked
and thus for the all Door instances. In other words, there will be complete tree for each instance.
--no-expand will result in a bit narrowed tree like:
Vehicle.CabinDoor.Row1/Row2.DriverSide/PassengerSide.IsLocked

Sorry for the bad formatting, but I am not used with this editor :)

Maybe, at the end, it would worth to keep things simple and just make sure that we have the --no-expand as default and "hide" this option from the user.

You, know what, I will do exactly this. We will make the vss2samm exporter to always load NOT EXPANDED VSS Tree, so we will handle the instances in proper way to avoid nodes duplication and further complications.

If, at later stage this --expand / --no-expand option is requested, we can always add it to the exporter.

Kostadin-Ivanov · 2024-09-11T09:02:22Z

Now, with regards to this PR.

Note that I just mentioned the diff I would like to apply but I did not apply it here. So the current diff does not change the handling of the instance thingy

I'd say that its result will be same as you explain with overlay deletion, but a bit shorter way i.e., instead of declaring each signal / branch to be "deleted", the user just specifies the list of signals he/she wants to have in the tree.

I understand. However, isn't that then also easy to do with things available? Just iterate over the tree and set delete: true to all nodes that are not in your list. Afterwards you can do:
tree.delete_nodes(findall(tree, filter_=lambda n: n.get_vss_data().delete))
I am still not sure why you need to know about the expanded state here.

Oh and if you think that it does not make sense that people are using --no-expand with the samm exporter: Not every exporter needs to support that option. You can then just get rid of the option and not pass it to get_trees or pass true to the expansion parameter.

Thank you for this hint, I will give it a try and see how it goes.

I am still not sure why you need to know about the expanded state here.

I used it to properly check whether selected signal is matching, because when tree is expanded we get the Vehicle....Row#.#Side... kind of path, but in the selected_signals we set selected signal paths without the Row / Side part.

When I refactor this, I will make sure that we can also handle and "expanded" paths.

sschleemilch · 2024-09-11T12:26:00Z

Vehicle....Row#.#Side... kind of path, but in the selected_signals we set selected signal paths without the Row / Side part.

Okay so it is about how the "selecting signals" syntax? #399 is merged, so maybe it will help you with that then. If any child has is_instance than you know that you are looking at an expanded branch

Kostadin-Ivanov · 2024-09-11T12:38:18Z

Vehicle....Row#.#Side... kind of path, but in the selected_signals we set selected signal paths without the Row / Side part.

Okay so it is about how the "selecting signals" syntax? #399 is merged, so maybe it will help you with that then. If any child has is_instance than you know that you are looking at an expanded branch

Thank you :)

I was looking for some kind of a proper identification, whether a branch is expanded, but somehow I've missed this one.

I will keep it in mind and get this managed in my next PR where I will refactor the filtering and related checks as you suggested in this discussion.

Maybe you can mark this PR as invalid, since I will set the default loading of the VSS Tree - --no-expand and will remove the need to check for is_expanded, at least for now :)

Kostadin-Ivanov · 2024-09-11T13:01:53Z

Sorry, @sschleemilch , I tried to find is_instance field on a vss_node, which I was sure that is it an instance and could not find such field.

The node I was looking at was Vehicle.Cabin.Door.Row1

Anyway, since we will default tree to be --no-expand we are good to skip these checks for the moment.

sschleemilch · 2024-09-11T13:03:52Z

it is not on a vss_node but on the data thingy

sschleemilch · 2024-09-11T13:05:47Z

Maybe you can mark this PR as invalid, since I will set the default loading of the VSS Tree - --no-expand and will remove the need to check for is_expanded, at least for now :)

Did you check the diff of this PR? It actually does nothing regarding that topic

Kostadin-Ivanov · 2024-09-11T13:12:00Z

it is not on a vss_node but on the data thingy

I could not find it there as well.

sschleemilch · 2024-09-11T13:45:12Z

it is not on a vss_node but on the data thingy

I could not find it there as well.

On a data object if it is a VSSDataBranch

Kostadin-Ivanov · 2024-09-12T06:06:32Z

Hello @sschleemilch ,

I am not sure how can I mark this as invalid, but it is "invalidated" from this PR: 404

Kostadin-Ivanov · 2024-09-12T06:15:43Z

it is not on a vss_node but on the data thingy

I could not find it there as well.

On a data object if it is a VSSDataBranch

I found it.

Not, sure how come I've missed it yesterday. Anyway, I will keep this in mind, for when, if :), we enable handling of --expand / --no-expand option in the vss2samm exporter.

sschleemilch · 2024-09-12T07:03:50Z

Hello @sschleemilch ,

I am not sure how can I mark this as invalid, but it is "invalidated" from this PR: 404

Can you please check the diff of this one? It is actually about something else: The dynamic imports

Kostadin-Ivanov · 2024-09-12T07:11:03Z

Hello @sschleemilch ,
I am not sure how can I mark this as invalid, but it is "invalidated" from this PR: 404

Can you please check the diff of this one? It is actually about something else: The dynamic imports

Sorry, I've missed the other changes. I will test your PR now and then I will approve.
Could you add me as reviewer?

Kostadin-Ivanov · 2024-09-12T07:29:12Z

src/vss_tools/vspec/vssexporters/vss2samm/config.py

+# Used in file_helper.write_graph_to_file to properly escape characters in filedata, before to write it to a file.
+
+
+class Config:


The Config file must be initialized in the vss2samm.py and before any other helper, which is using it, to be loaded.

Otherwise, when helpers are loaded before the initialization of the config, the OUTPUT_NAMESPACE, VSPEC_VERSION and SPLIT_DEPTH will be None, which will cause problems like: @prefix : <urn:samm:None:None#> . in the generated aspect models.

Expected result should be like: @prefix : <urn:samm:com.covesa.vss.spec:5..0.0#> .

This can be observed in the below comments.

Kostadin-Ivanov · 2024-09-12T07:33:35Z

src/vss_tools/vspec/vssexporters/vss2samm/helpers/namespaces.py

+ "samm": f"{samm_base_namespace}:meta-model:{Config.SAMM_VERSION}#",
+ "samm-c": f"{samm_base_namespace}:characteristic:{Config.SAMM_VERSION}#",
+ "samm-e": f"{samm_base_namespace}:entity:{Config.SAMM_VERSION}#",
+ "unit": f"{samm_base_namespace}:unit:{Config.SAMM_VERSION}#",
 }

 # Below formatted namespace should look like: urn:samm:com.covesa.vss.spec:5.0.0#


As per above, when this script is loaded, before the Config is initialized in the vss2samm, this will lead to @prefix : <urn:samm:None:None#> . in the generated aspect models, which will raise a SAMM error.

Expected result should be like: @prefix : <urn:samm:com.covesa.vss.spec:5..0.0#> .

The config, and more specifically: OUTPUT_NAMESPACE, VSPEC_VERSION and SPLIT_DEPTH fields must not be Null before config is loaded by any other script, i.e. we should init the config, and then dynamically load other helpers, which then will load concepts and namespaces, where the above config fields are used.

Kostadin-Ivanov · 2024-09-12T07:41:25Z

src/vss_tools/vspec/vssexporters/vss2samm/helpers/ttl_helper.py

@@ -148,7 +148,7 @@ def handle_branch_node(
 for child_node in vss_node.children:
 child_node_uri = None

- if split_vss and child_node.depth <= cfg.SPLIT_DEPTH and child_node.data.type is NodeType.BRANCH:
+ if split_vss and child_node.depth <= Config.SPLIT_DEPTH and child_node.data.type is NodeType.BRANCH:


At this stage, all of OUTPUT_NAMESPACE, VSPEC_VERSION and SPLIT_DEPTH are initialized i.e., they have values other than Null.

Kostadin-Ivanov · 2024-09-12T07:54:25Z

src/vss_tools/vspec/vssexporters/vss2samm/helpers/namespaces.py

 }

 # Below formatted namespace should look like: urn:samm:com.covesa.vss.spec:5.0.0#
 # and is used for the ":" bindings of the converted to TTLs, VSS Aspect models
 # that will refer to the user specified output_namespace
-samm_output_namespace = f"{samm_prefix}:{cfg.OUTPUT_NAMESPACE}:{cfg.VSPEC_VERSION}#"
+samm_output_namespace = f"{samm_prefix}:{Config.OUTPUT_NAMESPACE}:{Config.VSPEC_VERSION}#"


At this stage, all of OUTPUT_NAMESPACE, VSPEC_VERSION and SPLIT_DEPTH are not initialized i.e., they have Null values, which will result in: @prefix : <urn:samm:None:None#> .

sschleemilch · 2024-09-12T07:55:20Z

src/vss_tools/vspec/vssexporters/vss2samm/vss2samm.py

 from pathlib import Path

 import rich_click as click
 import vss_tools.vspec.cli_options as clo
+import vss_tools.vspec.vssexporters.vss2samm.helpers.ttl_helper as ttl_helper


i will close the PR now since I have no time fixing the root cause which in my opinion is using values statically that are generated dynamically...

Kostadin-Ivanov · 2024-09-12T07:56:57Z

src/vss_tools/vspec/vssexporters/vss2samm/helpers/samm_concepts.py

@@ -11,7 +11,7 @@

 from rdflib import URIRef

-from ..config import config as cfg
+from ..config import Config
 from . import string_helper as str_helper
 from .namespaces import samm_base_namespace, samm_output_namespace


At this stage, samm_output_namespace will be in the form: @prefix : <urn:samm:None:None#> .

Expected should be like: @prefix : <urn:samm:com.covesa.vss.spec:5..0.0#> .

Kostadin-Ivanov · 2024-09-12T08:00:32Z

src/vss_tools/vspec/vssexporters/vss2samm/vss2samm.py

-ttl_helper = None
-
-
-def __setup_environment(output_namespace, vspec_version, split_depth: int) -> None:


We will need to load these in this particular order. Otherwise, there could be some dependencies problems like no or wrong initialized fields related to the ones from above comments.

Kostadin-Ivanov · 2024-09-12T08:02:41Z

src/vss_tools/vspec/vssexporters/vss2samm/vss2samm.py

 vspec, include_dirs, aborts, strict, extended_attributes, uuid, quantities, units, types, overlays, expand
 )

 # Get the VSS version from the vss_tree::VersionVSS
 vss_version = __get_version_vss(tree)
-
- __setup_environment(output_namespace, vss_version, split_depth)
+ cfg.init(output_namespace, vss_version, split_depth)


At this stage VSSConcepts, vss_helper and tt_helper are loaded and any fields which depends on the user input, which is to be added to the initialization of the Config at this point will have Nulls in their values, as mentioned in above comments.

We should init the config, and then load the corresponding helpers etc.

Kostadin-Ivanov

Please check my comments above.

I tested this change and marked the places where Config fields were still in their Null values, which is causing problems in generated aspect models.

Please note that the python script completes OK, but the resultant aspect models have problem prefixes, like @prefix : <urn:samm:None:None#> . mentioned above.

Kostadin-Ivanov · 2024-09-12T08:15:19Z

i will close the PR now since I have no time fixing the root cause which in my opinion is using values statically that are generated dynamically...

Hello @sschleemilch , sure and thank you for catching this.

I will take a note and see if I we can find other way to dynamically provide the user input to these fields.

sschleemilch force-pushed the feature/cleanup-samm-exporter branch from cd899d8 to 31d1c0d Compare September 10, 2024 12:06

Fixed some samm exporter findings

387ad94

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>

sschleemilch force-pushed the feature/cleanup-samm-exporter branch from 31d1c0d to 387ad94 Compare September 10, 2024 12:24

Kostadin-Ivanov reviewed Sep 12, 2024

View reviewed changes

sschleemilch closed this Sep 12, 2024

Kostadin-Ivanov reviewed Sep 12, 2024

View reviewed changes

sschleemilch commented Sep 12, 2024

View reviewed changes

Kostadin-Ivanov reviewed Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎨 Fixed some samm exporter findings #402

🎨 Fixed some samm exporter findings #402

sschleemilch commented Sep 10, 2024 •

edited

Loading

erikbosch commented Sep 10, 2024

Kostadin-Ivanov commented Sep 10, 2024 •

edited

Loading

sschleemilch commented Sep 10, 2024 •

edited

Loading

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024 •

edited

Loading

Kostadin-Ivanov commented Sep 11, 2024 •

edited

Loading

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 12, 2024

Kostadin-Ivanov commented Sep 12, 2024

sschleemilch commented Sep 12, 2024

Kostadin-Ivanov commented Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading

sschleemilch Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading

Kostadin-Ivanov left a comment

Kostadin-Ivanov commented Sep 12, 2024

		# Used in file_helper.write_graph_to_file to properly escape characters in filedata, before to write it to a file.


		class Config:

		ttl_helper = None


		def __setup_environment(output_namespace, vspec_version, split_depth: int) -> None:

🎨 Fixed some samm exporter findings #402

🎨 Fixed some samm exporter findings #402

Conversation

sschleemilch commented Sep 10, 2024 • edited Loading

erikbosch commented Sep 10, 2024

Kostadin-Ivanov commented Sep 10, 2024 • edited Loading

sschleemilch commented Sep 10, 2024 • edited Loading

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024 • edited Loading

Kostadin-Ivanov commented Sep 11, 2024 • edited Loading

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 11, 2024

sschleemilch commented Sep 11, 2024

Kostadin-Ivanov commented Sep 12, 2024

Kostadin-Ivanov commented Sep 12, 2024

sschleemilch commented Sep 12, 2024

Kostadin-Ivanov commented Sep 12, 2024

Kostadin-Ivanov Sep 12, 2024

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

sschleemilch Sep 12, 2024

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Kostadin-Ivanov Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Kostadin-Ivanov left a comment

Choose a reason for hiding this comment

Kostadin-Ivanov commented Sep 12, 2024

sschleemilch commented Sep 10, 2024 •

edited

Loading

Kostadin-Ivanov commented Sep 10, 2024 •

edited

Loading

sschleemilch commented Sep 10, 2024 •

edited

Loading

Kostadin-Ivanov commented Sep 11, 2024 •

edited

Loading

Kostadin-Ivanov commented Sep 11, 2024 •

edited

Loading

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading

Kostadin-Ivanov Sep 12, 2024 •

edited

Loading