Implement VSC AST using python dataclasses

[miketsukerman]

Vehicle Service Catalog AST using python dataclasses

The general idea is to use https://docs.python.org/3/library/dataclasses.html instead of a python dictionary to describe the types from which an AST would consist.

Example code

Example python code for the method element. (https://github.com/COVESA/vehicle_service_catalog/blob/master/syntax.md#namespace-list-object-methods)

methods:
    - name: move
      description: Set the desired seat position
      input: 
         - name: seat
           description: The desired seat position
           datatype: movement.seat_t

will correspond to the following python code:

@dataclass
class Method:
    name: str
    description: Optional[str] = None
    error: Optional[List[Error]] = field(default_factory=EmptyList)
    input: Optional[List[Argument]] = field(default_factory=EmptyList)
    output: Optional[List[Argument]] = field(default_factory=EmptyList)

The fully AST python code is in the vsc_ast.py.

Reading AST from YAML file

from vsc.model import vsc_parser
service = vsc_parser.get_ast_from_file('input.yaml')

Building AST in the code

from vsc.model import vsc_ast
service = vsc_ast.AST(name='test', description='test', major_version=1, minor_version=0)

Additional changes

The PR contains additional changes. During the work on the dataclasses AST, the vsc-tools python module structure has been improved. Now we have vsc module with generators, model, scripts, templates submodules. vsc_ast.py contains AST specification implemented using python dataclasses. vsc_generator.py is responsible for the code generation. vsc_parser.py provides API to load and parse YAML files into AST classes.

├── LICENSE
├── Pipfile
├── Pipfile.lock
├── README.md
├── requirements.txt
├── setup.py
├── tests
│   ├── gen_test.py
│   ├── __init__.py
│   ├── input.yaml
│   ├── result
│   └── template
└── vsc
    ├── generators
    │   └── __init__.py
    ├── __init__.py
    ├── model
    │   ├── __init__.py
    │   ├── vsc_ast.py
    │   ├── vsc_generator.py
    │   └── vsc_parser.py
    ├── scripts
    │   ├── generator.py
    │   └── __init__.py
    └── templates
        ├── Argument-simple_doc.html
        ├── AST-simple_doc.tpl
        ├── dtdl.md
        ├── dtdl.tpl
        ├── Error-simple_doc.html
        ├── __init__.py
        ├── Member-simple_doc.html
        ├── Namespace-simple_doc.html
        ├── protobuf.md
        ├── protobuf.tpl
        ├── sds-bamm-aspect-model.md
        ├── sds-bamm-aspect-model.tpl
        ├── sds-bamm-macros.tpl
        ├── Service-simple_doc.html
        ├── simple_overview.tpl
        └── test.tpl

We added setup.py to simplify the installation of the module using pip or python3 setup.py install (python3 setup.py develop).

Installation from git

pip3 install git+https://github.com/COVESA/vsc-tools.git

Command line tools

Newly added setup.py will install a command line tool. The tool called vscgen can be used to generate the code using default templates or custom templates.

vscgen <path-to-yaml-file> <template-file-name>

The list of templates can be found in the README.md.

Mikhail Tsukerman [email protected] on behalf of MBiTION GmbH.

https://github.com/mercedes-benz/daimler-foss/blob/master/PROVIDER_INFORMATION.md

[gunnarx]

First I cherry-picked and pushed e8ec7ee because it seems to be needed for the GH actions to work now.
(I found this out in PR #31).

But now a new failure.

@miketsukerman Could you take a look at the buildtest failure now? It looks like the import statement fails.
Did you run gen_test.py manually (or run pytest?) because I can't make it work from any directory but I only did a few quick tests.

Some hints: Before your changes in f0d886a I had a bit of a hack to set the search path before import: (see HERE)
Maybe you can find a better way and still make it work?

[miketsukerman]

First I cherry-picked and pushed e8ec7ee because it seems to be needed for the GH actions to work now. (I found this out in PR #31).

But now a new failure.

@miketsukerman Could you take a look at the buildtest failure now? It looks like the import statement fails. Did you run gen_test.py manually (or run pytest?) because I can't make it work from any directory but I only did a few quick tests.

Some hints: Before your changes in f0d886a I had a bit of a hack to set the search path before import: (see HERE) Maybe you can find a better way and still make it work?

I run the test from pycharm with dedicated python virtual environment.

# This is maybe not ideal way but seems efficient enough
from pathlib import Path
import sys
proj_root = Path(__file__).parents[1]
sys.path.append(str(proj_root) + "/model")

Should not be needed in general. I was asking myself why this repository doesn't have setup.py? I think if setup.py would be added should eliminate the problem.

[gunnarx]

@miketsukerman In addition to what we talked about (not parsing some of the tree correctly) I saw today that it is missing
Enumerations. link to spec Did you plan to add them later (they are a little tricky I know)

EDIT: Actually lots more. .... the Namespace class is missing structs, methods, events, properties, includes?

I had just assumed you would take the previous code and convert from there. Because I went through the whole specification in detail when I wrote it and made sure the "ast definition" was as close as I could manage. So I recommend reusing the "schema" and just convert it to the classes. ^^^
We might look over the optionality once more however.

It will be hard to test this before we have the AST classes close to complete, I think.

Will you do that update, please?

[erikbosch]

I am not a pip expert, but previously for VSS I got the impression that it is good practice to always have a Pipfile.lock as well as the Pipfile to "lock" the build to specific versions. Or do we see that as not needed here?

[miketsukerman]

I will bring it back later. There were issues with pipenv on Linux. During the experiments I have removed it.

[miketsukerman]

@erikbosch I have reverted Pipenv.lock file back, please check and resolve the thread.

[erikbosch]

Is this a test to support the "old" syntax (without explicit "service"). Do we intend to support both or is this just a temporary test?

[miketsukerman]

We are in the process of migration and I've kept both. But, in the end, only one syntax will be left.

[miketsukerman]

@erikbosch I have reverted most of the changes I've made with "syntax". Some minor things are still there, like input instead of in, etc., but it is not avoidable.

[gunnarx]

@erikbosch Honestly it's all a bit of a mess at the moment! While merging and updating some of my code on top of this I noticed we had a mix of proposed syntax changes from @miketsukerman, a few from myself, the templates, the specification, different implementations in progress, unit-tests, examples files in VSC repo - all had various levels of differences. To get a stable situation I have also rolled back a lot, to a syntax that I don't feel is the final/best one.
But I'd rather we start from there and change incrementally. From now on, let's try to make sure all PRs will modify parts in sync. (This is still Draft PR of course, but nonetheless).

[miketsukerman]

@gunnarx let's call it refactoring :)

[gunnarx]

Yes these latest commits are similar changes I had to make…. Also, we already chatted about this but just to document: I think the updated parsing function will instantiate all parts immediately so the post init functions here won’t be necessary. I will find some time later tomorrow to compare and merge it all once again, and then we will see if it works as expected.

[miketsukerman]

Yes these latest commits are similar changes I had to make…. Also, we already chatted about this but just to document: I think the updated parsing function will instantiate all parts immediately so the post init functions here won’t be necessary. I will find some time later tomorrow to compare and merge it all once again, and then we will see if it works as expected.

yes, that would be great. If it works with templates we are good to go I guess.

[gunnarx]

@miketsukerman We can't work like this... There's no way to reasonably merge changes together when you have commits like this recent push. It was a problem before as well that things were not separated the right way, and the squashing together of commits is rarely the solution to bad commits - it is likely to create even worse commits.

Too many of these commits contain too many changes that are not related to each other. The effect of this is that they can't be used separately or effectively combined with other changes without lots of merge conflicts. (b6fa1af is only one example).

Basically, I wonder why you didn't continue the work on top of my changes? Do you think it's better to just do this on your own?

There are even deeper issues here that I can explain separately, but before I go too far - is all this just because you need some help to know how to separate things in git, or are you kind of just developing without regard for others?

[miketsukerman]

@miketsukerman We can't work like this... There's no way to reasonably merge changes together when you have commits like this recent push. It was a problem before as well that things were not separated the right way, and the squashing together of commits is rarely the solution to bad commits - it is likely to create even worse commits.

Too many of these commits contain too many changes that are not related to each other. The effect of this is that they can't be used separately or effectively combined with other changes without lots of merge conflicts. (b6fa1af is only one example).

Basically, I wonder why you didn't continue the work on top of my changes? Do you think it's better to just do this on your own?

There are even deeper issues here that I can explain separately, but before I go too far - is all this just because you need some help to know how to separate things in git, or are you kind of just developing without regard for others?

I am doing experiments in this MR. I think, the idea to have two MRs in parallel is not so gut. I am sorry that I have disappointed you with the way how I approach this. For my excuse I can tell that I want to explore all the things first, then I can prepare the changes in, maybe, a new clean MR.

Would be better if we will have one MR, for example, this one and you will comment on it and I will fix it simply. Rebasing for me onto your MR is difficult as well.

When we will solve all the issues, I will open one smaller MR.

[gunnarx]

1. The idea to have the type as AST came from the original implementation where all node-types were sub-classes inheriting from the AST class. Thus the type of node "is-a" AST. But that class hierarchy does not exist anymore.

2. Even if the top-level node will be Namespace, the purpose of the gen() function is that it can accept any node type.

So... in this version where the common type (parent class) AST does not exist, I would guess that it is just as good to not give a type hint at all.

[miketsukerman]

1. Once you mentioned that you wanted to make everything according to the specification and in the specification, there is no root element except Namespace. No problem, I will fix it later today and bring AST back.

2. Hm, for this purpose in python there is a generic type, something like that would be

https://docs.python.org/3/library/typing.html#typing.Generic

from dataclass import is_dataclass
from typing import TypeVar, Generic

T = TypeVar['T']

def gen(node: Generic[T], template_file = None):
    assert is_dataclass(node)

I will try this, may be a good idea to write this function like that, but check the passed types for some criteria.

[miketsukerman]

@gunnarx done, you can check.

[miketsukerman]

Well, just a thought

def gen(node: Any, template_file = None):
...

would work

[gunnarx]

1. The idea to have the type as AST came from the original implementation where all node-types were sub-classes inheriting from the AST class. Thus the type of node "is-a" AST. But that class hierarchy does not exist anymore.

OK... point 1 was just information, not really a change request. As I said the class hierarchy does not exist now, so therefore there is (was) no AST parent type. However 2. suggests that the gen() function should have a different type, like Any since we don't have a more specific common parent type anymore.

1. Once you mentioned that you wanted to make everything according to the specification and in the specification, there is no root element except Namespace. No problem, I will fix it later today and bring AST back.

Yes of course I wanted the behavior to be according to specification. So I understand why that led you to put Namespace in some places, I'm just providing feedback and explanation on where I think the change makes sense and not.

Well, just a thought

def gen(node: Any, template_file = None):
...

• Any seems correct with what we have now.

[miketsukerman]

OK... point 1 was just information, not really a change request. As I said the class hierarchy does not exist now, so therefore there is (was) no AST parent type.

I know. I was not sure completely, waiting for your feedback.

[gunnarx]

I would still say that the whole thing that is provided into the generator is an Abstract Syntax Tree, even if in the current syntax definition the top level node type is Namespace.

I guess in this comment the word AST could be considered the concrete class name, but it could also be considered the "concept" of providing an AST from the parser. I would personally keep the name AST.

[miketsukerman]

I've returned AST node back.

@dataclass
class AST(Namespace):
    pass

[miketsukerman]

@gunnarx some topics to think about:

1. Do we need code below in the vsc_parser.py? We have vscgen instead.

# If run as a script, generate a single YAML file using a single root template
if __name__ == "__main__":
    if not len(sys.argv) == 3:
        usage()
    ast = vsc_ast.read_ast_from_yaml_file(sys.argv[1])
    templatename = sys.argv[2]
    print(gen(ast, templatename))

2. I would like to move some code from the vsc_generator.py to templates module

# Set up Jinja
jinja_env = jinja2.Environment(
        # Use the subdirectory 'templates' relative to this file's location
        loader=jinja2.FileSystemLoader(TemplatePath),

        # Templates with these extension gets automatic auto escape for HTML
        # It's more annoying for code generation, so passing empty list for now.
        autoescape=jinja2.select_autoescape([])
        )

# We want the control blocks in the template to NOT result in any extra
# white space when rendering templates. However, this might be a choice
# made by each generator, so we need to export the ability to keep these
# settings public for other code to modify them.
jinja_env.trim_blocks = True
jinja_env.lstrip_blocks = True
jinja_env.undefined = jinja2.StrictUndefined

default_templates = {}

That would make vsc_generator.py more abstract.

[miketsukerman]

Meeting 19 September: OK to merge after adding appropriate license-headers and signoffs.

Updated PR description with short summary. Added license headers etc.