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

Core Rework ⚠️ BREAKING CHANGES #382

Merged
merged 22 commits into from
Aug 20, 2024

Conversation

sschleemilch
Copy link
Collaborator

@sschleemilch sschleemilch commented Jul 19, 2024

Fixes: #380
Fixes: #390
Fixes the issue in discussion of #360 (removed the test in test_overlay_on_instance)

Rewrote the core logic using Pydantic.

  • Tree nodes contain a Pydantic class instance in their 'data' attribute
  • Pydantic handles the model validation
  • Model is initialized in 'relaxed' mode so that we can support
    overlaying of instance nodes without the need to specify nodes
    completely
  • Added a lot more validations to the model regarding consistency of
    datatypes, units, allowed attributes etc
  • Also added tree validation regarding what node types can have what types of parents (all nodes need to have branches as parents except properties). Enforcing it
  • Rewrote include vspec logic and more
  • Rewrote instance expansion logic and more
  • Adapted exporters and improved slightly (e.g. csv uses the csv lib)
  • Restricting -e/--extended-attributes to not be able to pass core attributes

Tip

Overlay nodes do not need any attributes anymore except the ones you want to overwrite
That is also valid for overwriting instances.
Example:

Vehicle:
  type: branch
  description: Vehicle

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

Vehicle.Door.Row1:
  description: Very special Door

Warning

This breaks the backwards compatibility to older vss versions and therefore the test has been marked to be skipped.
Reason is that we handle unit and quantity files more strict and because of that, vss had to be patched (COVESA/vehicle_signal_specification#758). Old release are decided not to be patched

Points of interest / Questions / Code TODOs

  • We are never exporting the arraysize attribute. On purpose? 🆗 Not on purpose but we keep not exporting it for now until needed
  • vss2binary -> export_node -> validate what is it? 🆗 Clarified. In future to be discussed whether it should be part of the model when exporters explicitly mentioning it.
  • Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior? 🆗 Type tree does not need to follow name style conventions. It would lead to weird property names

Added

  • Exporter to render a tree (vspec export tree)
    image

Todo

  • Tests for newly introduced validations
  • Inline docs
  • Add has as valid bool prefix

⚠️ Behavior changes to be discussed -> Change is ok 🆗

Extending instances implicitly

Take this config

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

Vehicle.Door.Row3:
  type: branch
  description: Got another door

Vehicle.Door.Id:
  type: branch
  description: Door ID

Although there are only Row1 and Row2 created the user wants to add another one implicitly.
However that is some weird magic since it is unlogical to distinguish the behavior from what is displayed in the config: Adding something to all instances. Just because the name of Row3 matches the instance expansions we should not rely on that imo.

So, the outcome of the config as is would be in this implementation:

Vehicle
└── Door
    ├── Row1
    │   ├── Row3
    │   └── Id
    └── Row2
        ├── Row3
        └── Id

That is obviously not what the config intended but in my opinion the config is incorrect. When changing instances to Row[1,3] we get the correct result:

Vehicle
└── Door
    ├── Row1
    │   └── Id
    ├── Row2
    │   └── Id
    └── Row3
        └── Id

Validation improvements

Cross checking of allowed-datatypes

Taking this example config:

kWh:
  definition: Energy consumption measured in kilowatt hours
  unit: kilowatt hours
  quantity: work
  allowed-datatypes: ['numerics']

Is detected as a misconfiguration since numerics does not exist but passes the current vss-tools model:
image

Cross checking of quantity being defined

Pretty similar config but using a quantity that is not defined:

kWh:
  definition: Energy consumption measured in kilowatt hours
  unit: kilowatt hours
  quantity: workie
  allowed-datatypes: ['numeric']

Is detected by tool but not by vss-tools:
image

Strict units yaml structure

Protecting against wrong structure in units.yaml and quantities.yaml such as using description instead of definition, units instead of unit
We actually have already violations in the current vss:

diff --git a/spec/units.yaml b/spec/units.yaml
index 5f32a026..ee0c9ab3 100644
--- a/spec/units.yaml
+++ b/spec/units.yaml
@@ -58,7 +58,7 @@ cm/s^2:
 # Volume

 ml:
-  description: Volume measured in milliliters
+  definition: Volume measured in milliliters
   unit: milliliter
   quantity: volume
   allowed-datatypes: ['numeric']
@@ -138,7 +138,7 @@ g:
   allowed-datatypes: ['numeric']
 kg:
   definition: Mass measured in kilograms
-  label: kilogram
+  unit: kilogram
   quantity: mass
   allowed-datatypes: ['numeric']
 lbs:
@@ -205,7 +205,7 @@ weeks:
   allowed-datatypes: ['numeric']
 months:
   definition: Duration measured in months
-  units: months
+  unit: months
   quantity: duration
   allowed-datatypes: ['numeric']
 years:

Cross checking arraysize to datatype being an array

Example config

Vehicle:
    type: branch
    description: vehicle

Vehicle.Test:
    type: sensor
    datatype: uint8
    arraysize: 3
    description: foo

Is detected by the tool (note that datatype is not an array) but vss-tools has no problem with it:
image

Similar: Also checks that arraysize matches the entries of default:
image

Checking default against datatype

Example:

Vehicle:
    type: branch
    description: vehicle

Vehicle.Test:
    type: sensor
    datatype: uint8[]
    description: foo
    default: 3

Is detected but is not currently:
image

Checking allowed entries matching datatype

Similar to the use case before, Config:

Vehicle.Test:
    type: sensor
    datatype: uint8[]
    arraysize: 3
    description: foo
    allowed: [1,2,'foo']

Note that foo is not an uint8 and should not be accepted.
Is detected:
image

It also checks ranges e.g. -3 will violate uint8.

Checking min/max set together with allowed

Config:

Vehicle.Test:
    type: sensor
    datatype: uint8
    description: foo
    min: 3
    max: 5
    allowed: [2,3]

Result:
image

Checking of default values against allowed

Config:

Vehicle.Test:
    type: sensor
    datatype: uint8
    description: foo
    allowed: [1,2,3]
    default: 4

Result:
image

Checking of datatype vs allowed_values in specified unit

Config:

Vehicle.Speed:
    type: sensor
    datatype: string
    unit: kWh

Result:
image

@sschleemilch sschleemilch marked this pull request as draft July 19, 2024 14:32
@sschleemilch sschleemilch force-pushed the feature/core-rework branch 7 times, most recently from 7aa37eb to 50ad785 Compare July 29, 2024 06:48
@sschleemilch sschleemilch force-pushed the feature/core-rework branch from 3718d7b to ac50f5a Compare July 29, 2024 09:44
@sschleemilch sschleemilch changed the title WIP Feature/core rework WIP Feature/core rework ⚠️ BREAKING CHANGES Jul 29, 2024
@sschleemilch sschleemilch force-pushed the feature/core-rework branch from 24184fc to ddf228b Compare July 30, 2024 05:41
@erikbosch
Copy link
Collaborator

MoM:

  • Feature complete, please review, but minor changes may still happen
  • Remaining works is to fix unit test and address bugs found in testing

@sschleemilch sschleemilch force-pushed the feature/core-rework branch 2 times, most recently from 3e9d96e to e060807 Compare July 31, 2024 06:46
if csv_field is None:
csv_field = ""
formatted_csv_line = (
formatted_csv_line + '"' + str(csv_field).replace('"', '""') + '",'
Copy link
Collaborator

@erikbosch erikbosch Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice some changes in output of CSV exporters. This line was added once upon the time to handle at least some common scenarios, like if a description/comment contain a comma or a double quote, and then the string has to be quoted according to https://datatracker.ietf.org/doc/html/rfc4180

Now it seems we skip any form of escaping, or?

(Or are you compensating it by adding some stricter checks, like an error if someone use a comma in a description?, or just removing such characters.)

An alternative solution could possibly be to only use "default escaping" only for free text fields like description and comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the csv writer library handles escaping for us. And yes, the output of all csv's changed and removed unnecessary quoting

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I will check some corner cases I guess

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be quite easy to add some test cases unless we already have some.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works fine. I honestly see no real reason to test the official csv lib

@sschleemilch sschleemilch force-pushed the feature/core-rework branch 7 times, most recently from 5c76cd2 to 2deb3f8 Compare August 2, 2024 13:17
@sschleemilch sschleemilch changed the title WIP Feature/core rework ⚠️ BREAKING CHANGES Core Rework ⚠️ BREAKING CHANGES Aug 2, 2024
@sschleemilch sschleemilch marked this pull request as ready for review August 2, 2024 13:31
@sschleemilch sschleemilch force-pushed the feature/core-rework branch 4 times, most recently from d599b92 to 7b71f20 Compare August 5, 2024 10:17
@sschleemilch sschleemilch force-pushed the feature/core-rework branch 2 times, most recently from ea137af to 178295c Compare August 6, 2024 11:26
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
@erikbosch
Copy link
Collaborator

MoM:

#Questions

We are never exporting the arraysize attribute. On purpose?

S Schildt: Not on purpose

vss2binary -> export_node -> validate what is it?

@UlfBj to be the one to answer.

Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior?

  • E: Does anyone used struct concept

  • Adnan will check with S Schleemilch what it should be

  • Please review

  • Possible merge decision next week

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
@sschleemilch
Copy link
Collaborator Author

MoM:

#Questions

We are never exporting the arraysize attribute. On purpose?

S Schildt: Not on purpose

vss2binary -> export_node -> validate what is it?

@UlfBj to be the one to answer.

Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior?

  • E: Does anyone used struct concept
  • Adnan will check with S Schleemilch what it should be
  • Please review
  • Possible merge decision next week
  • vss2binary validate feature is fixed
  • not changing export behavior for now regarding arraysize
  • not validating names for the types tree

From my perspective, it is done

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Changed how includes are resolved since it was non deterministic.
Now building a complete list of all vspec files first.
Merging all of them afterwards.
Changed that dict values are deeply updated.

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
@UlfBj
Copy link
Contributor

UlfBj commented Aug 8, 2024

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to:
More information can be found in: VISS Access Control.

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
@sschleemilch
Copy link
Collaborator Author

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to: More information can be found in: VISS Access Control.

I am wondering why it is not a specified internal attribute if it is present in the code and explicitly mentioned?

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
set does not guarantee an order.
However an order is crucial for something like include dirs
a user can specify.
Files could exist in two include dirs.

Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
@SebastianSchildt
Copy link
Collaborator

Erik: @UlfBj can you check if the field validate is described somewhere? Extend https://github.com/COVESA/vss-tools/blob/master/binary/README.md with an example or formal syntax stating that it is a string.

This information is already linked to: More information can be found in: VISS Access Control.

I am wondering why it is not a specified internal attribute if it is present in the code and explicitly mentioned?

Because it is "only" used in VISS (and thus the binary exporter, which currently is also only for the VISSR reference implementation), and not considered part of VSS (i.e. expected to be supported by all VSS tooling). That does not mean it is "not good", it is just similar to e.g. DBC mapping keys (currently afaik only used in KUKSA) or some specific deployment keys BMW might use in internal tooling.

I think, what would be a nice addition for the future, in case a specific tooling "requires" or understands special keys (like the binary exporter), if it could programmatically via an API whitelist those, instead of forcing the user to whitelist them on the command line.

@SebastianSchildt
Copy link
Collaborator

Meeting 20th: Looking good,

What about the breaking changes?

  • backport unit changes to old VSS releases?
    • Seb: maybe not
  • old tooling is still working on master
  • just warn?
  • will just postpone the problem

MoM: Merge

@SebastianSchildt SebastianSchildt merged commit 3910c27 into COVESA:master Aug 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants