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

v4.2.1: Mandatory Description missing in implicitly created branches in vspec2x #407

Open
mikehaller opened this issue Sep 27, 2024 · 6 comments

Comments

@mikehaller
Copy link

I have a "broken" .vspec file where the nodes are not correctly ordered, e.g. a sub-branch comes before its parent branch.

That causes the vss-tools to "implicitly auto-creating branches", which seems to be a really nice feature.
However, since at the time where the branch node is implicitly created there is no description given, subsequent steps error out.

Expanding direct VSpec to expanded JSON.../generated/no-covesa/vspec/Vehicle/Cabin/Hvac/Fronthousing.vspec
INFO     VSS-tools version 4.2.1
INFO     Known extended attributes: dbcinfo, dbc, dbc2vss, vss2dbc
INFO     No quantities defined!
INFO     Added 52 units from /generated/no-covesa/vss_custom_units.yaml
INFO     Loading vspec from /generated/no-covesa/vspec/Vehicle/Cabin/Hvac/Fronthousing.vspec...
INFO     Autocreating implicit branch Vehicle
INFO     Autocreating implicit branch Cabin
INFO     Autocreating implicit branch Cabin
INFO     Autocreating implicit branch Hvac
ERROR    Invalid VSS element Cabin, must have description

Tested with v4.2.1

This code here is creating a new node, but does not set the required field description:
https://github.com/COVESA/vss-tools/blob/release/4.2/vspec/__init__.py#L685

Did not yet test with main branch, as I couldn't find that function.
May be obsolete.

@erikbosch
Copy link
Collaborator

The implicit branches creation was intended primarily for overlays to avoid the need to add a branch that isn't changed in an overlay just for the sake of it. But it can also be used for cases where the branch does not exist since before and there we need to make up our mind, either make sure it works or create a proper error message like "Implicit branches only allowed in overlays" or so.

VSs-tools has recently been refactored so the error is likely produced elsewhere in the latest version - this is what you get currently with an implicit node.

Thanks for reporting, we should better look into it.

Note to the community:

We cannot just add a default value when we create an implicit node as I then think that note will get priority when we apply an overlay. We could discuss if we should instead relieve the requirement to have a description for branches - it is maybe not that useful in all cases.


erik@debian6:~/vehicle_signal_specification$ vspec export yaml --vspec implicit.vspec -o vss.yaml
[12:40:56] WARNING  No 'quantity' files defined. Default not existing:                                         main.py:60
                    /home/erik/vehicle_signal_specification/quantities.yaml                                              
           WARNING  No 'unit' files defined. Default not existing:                                             main.py:66
                    /home/erik/vehicle_signal_specification/units.yaml                                                   
           INFO     VSpecs loaded, amount=1                                                                  vspec.py:122
           CRITICAL 'A.B' has 1 model error(s):                                                               main.py:238
                    [                                                                                                    
                        {                                                                                                
                            'type': 'missing',                                                                           
                            'loc': ('description',),                                                                     
                            'msg': 'Field required',                                                                     
                            'input': {'fqn': 'A.B', 'type': 'branch'}                                                    
                        }                                                                                                
                    ]                                                                                                    
erik@debian6:~/vehicle_signal_specification$ 

@sschleemilch
Copy link
Collaborator

sschleemilch commented Sep 27, 2024

yes, I specifically did not support auto creation of branches. It is not a big deal to support it though if that is what we want. Or to be more specific: Branches are created implicitly but are not prefilled with a description and therefore do not comply to the final model. However, it makes overlays more powerful since you do not need to define any additional things in the overlay if the final node has the right format after applying all overlays. That is something that earlier vss-tools could not support in their design.

@erikbosch
Copy link
Collaborator

Let us discuss on Tuesday how we want to handle it, i.e. if we want to allow implicit branches also in "final model".

@erikbosch
Copy link
Collaborator

MoM:

  • Ulf: I am not in favor for allowing impliit branches (without description) (except for overlays)
  • Sebastian Schildt: Do not use dummy descriptions
  • Erik: Please discuss, lets agree next week

@erikbosch
Copy link
Collaborator

Trying to list out the alternatives that I see using the example below.

  1. Only allow implicit branches for overlays where the branch exist somewhere else already. In this case we should better get an explicit error "Branch A.B must be defined, implicit branches not allowed in final model.", rather than the complaint about that it has no description.
  2. Allow it, but then as Sebastian suggests allow empty descriptions for implicit branches (or for all branches, they are not always that useful today)
A:
  type: branch
  description: Branch A.


A.B.C:
  datatype: int8
  type: sensor
  unit: km
  description: This is C

@erikbosch
Copy link
Collaborator

MoM:

  • Ulf: Prefer alt 1 (i.e give better error)
  • Decision: For now stick with alt 1, i.e. do not allow implicit branches in final model, but possibly give better error message

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

No branches or pull requests

3 participants