:art: Fixed some samm exporter findings

[sschleemilch]

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

[erikbosch]

MoM:

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

[Kostadin-Ivanov]

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.

[sschleemilch]

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 (b8985624c1)

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]

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]

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]

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

[Kostadin-Ivanov]

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:

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

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

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]

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? https://github.com/COVESA/vss-tools/pull/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]

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]

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]

it is not on a vss_node but on the data thingy

[sschleemilch]

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]

it is not on a vss_node but on the data thingy

I could not find it there as well.

[sschleemilch]

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]

Hello @sschleemilch ,

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

[Kostadin-Ivanov]

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]

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]

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]

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.