Implement fbs schema & json to fbs converter tool - #738
Conversation
a2ca224 to
2510ac0
Compare
|
As discussed via Slack - I love that we go for the Flatbuffers support and the contribution you perform here! But it would be very welcome to see the roadmap and map for its qualification before continuing here. Stating again: This does not require a full and ready qualification, we would just like to judge if the qualification at the end can succeed. |
Hi Jan, I agree. |
castler
left a comment
There was a problem hiding this comment.
Thanks this is going into the right direction.
I still believe that we need a flatbuffers certifcation for the 1.0 - and not only in 2027. And we should have a parallel workstream with weekly (or bi-weekly) reporting about the tasks there.
Anyhow - for this PR, my major concern is that we know have 4 places where we need to know the different schemas:
- In the JSON Parser (soonish also flatbuffers parser +1)
- In the JSON Schema
- In the Flatbuffers Schema
- In the JSON to Flatbuffers converter
I think this is to much duplication and will make it not maintainable. We should strive to have a generic flatbuffers schema generatioin and also a generci json to flatbuffers converter - to reduce the duplication as much as possible.
|
|
||
| # Enables FlatBuffers-based mw::com configuration tooling (disabled by default, | ||
| # see score/mw/com/impl/configuration/flatbuffers_flags.bzl). | ||
| common:flatbuffers --//score/mw/com/impl/configuration:enable_flatbuffers=true |
There was a problem hiding this comment.
Can we please name this experimental_enable_flatbuffer_configuration
| //score/mw/com/impl/configuration/converter:json_to_flatbuffer | ||
| //score/mw/com/impl/configuration/converter:mw_com_config.bin |
There was a problem hiding this comment.
We should guard this via the experimental flag - and since the experimental flag is per default off, these targets should net yet be publicly visible.
There was a problem hiding this comment.
Is it okay if I remove them from here and add them to EXCLUDED_PUBLIC_TARGETS in quality/visibility_guard/parser.py?
There was a problem hiding this comment.
No, as Long as it is Experimental it should Not be public
| the pinned ``@flatbuffers//:flatc`` compiler, so ``.bin`` artifacts become part | ||
| of the build graph and can be consumed by other targets (e.g. as ``data``). | ||
| """ | ||
|
|
There was a problem hiding this comment.
you are missing a visibility declaration in this *.bzl file
There was a problem hiding this comment.
I'll add it as
visibility(["//score/mw/com/impl/configuration/..."])
| // | ||
| // FlatBuffers schema mirroring the mw::com runtime configuration. | ||
| // | ||
| // This schema is the binary-serialization counterpart of the JSON configuration |
There was a problem hiding this comment.
I would have expected that we generate this file out of the JSON schema - otherwise, we get a problem with maintainability - as we have to take care in various places the the configuration is maintained.
There was a problem hiding this comment.
That would not be safe, JSON Schema is loosy and defines only the structure.
1- It doesn't specify the size (Integer widths: uint8, 16, ...)
For an ASIL / safety-relevant config, silently inferring a wrong width (e.g. int64 where the C++ side expects uint16) is exactly the kind of defect that wouldn't be caught by flatc.
2- Enum sentinels e.g. INVALID = 0 is a deliberate "unset detection" (design mirroring) for QualityType
3- = null optional-scalar vs baked default
4- Table names for nested objects (e.g. ServiceTypeEvent vs InstanceEvent)
5- json_to_flatbuffer.py hardcodes the same key->field mapping
So yes, maintaining 2 Schemas is not great, but it's the safest way here (with existing one common testing for both - to come in next stories)
Also, JSON Schema is not changing that often, mostly stable, right?
What I suggest, is to implement a "drift-detection test" that parses the JSON schema and the .fbs and fails the build if their structure diverges.
This catches if someone edits the JSON schema and forgets the .fbs.
What do you think?
There was a problem hiding this comment.
Its not only the two schemas, we will also have another instance of this mapping in the json to flatbuffer tool.
This will be (hopefully) a long-term solution, if flatbuffers is safety certified, and thus it will be a maintaince nightmare. I agree that the config does not change often, but it will change - and then you do not want to edit so many places.
I see multiple options:
1st) Have integers always as uint64. We have the very same problem with JSON and there we check the boundary values within the C++ code. If we go this way, it is the same mechanichs in JSON ant Flatbuffers. Same is true for enum sentinels or "null optional scalars. We should just handle both the same and not have defaults in the one, where we have optional values in the other
2nd) Generate the JSON schema from flatbuffers. As per my understanding this is directly supported by flatc. <- But this will be though since this will only work with "experimental flag turned on" most certainly?
There was a problem hiding this comment.
I'll investigate the second option as I see it more robust.
There was a problem hiding this comment.
I think we can have the JSON schema committed to the repo, and when a speial flag is on then it will be regenerated and overwritten, but this would be needed only with Schema change/update.
Then we will not need to maintain both schemas manually, but just the Flatbuffers schema and regenerate the JSON one.
Would this be okay if feasible?
There was a problem hiding this comment.
yes that would be better IMHO
There was a problem hiding this comment.
Ok, here's some analysis limitations for using fbs schema as our main schema to maintain and generate the JSON schema from it using 'flatc --jsonschema':
- Hyphen '-' is forbidden in fbs, so I need to convert it to '_' for example, and restore it when generating the JSON schema (doable)
- flatc --jsonschema output can't reproduce the rich hand-authored JSON schema (it loses titles/descriptions/defaults/min-max/string-enums)
To not tradeoff here, I'd try adding custom attributes to fbs and try writing a script that appends these attributes to the generated JSON Schema (will be much of code but if once done, would save the maintenance nightmare you mentioned)
I'll ping you once I have more updates
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
| """Convert an mw::com configuration JSON file into a FlatBuffer binary. |
There was a problem hiding this comment.
My hope is, that if we auto generate the *.fbs, we also get rid of this tool (or make it way more easy - as we do not have yet another place where we translate between the two tools
There was a problem hiding this comment.
This converter tool is only a facilitating option for whom already has a JSON config and wants to use flatbuffer config binaries for its benefits.
It's converting to flatbuffer convertable json format (snake case)
User has the freedom to use any other method to get flatbuffer binaries directly.
For example:
- Programmatic builder (FlatBufferBuilder in C++, the flatbuffers package in Python, etc.) with any other text formats: YAML, TOML, XML, CSV, .ini.
- A database or network service — query rows/records and build directly.
- In-memory C++ objects
There was a problem hiding this comment.
I understand that, but Here we have then again the key mapping. Thats why auch a tool ja fine, but it should ne generic without hardcoding again the mappings
Thomas-Mikhael
left a comment
There was a problem hiding this comment.
I added my replies/questions for each comment
| // | ||
| // FlatBuffers schema mirroring the mw::com runtime configuration. | ||
| // | ||
| // This schema is the binary-serialization counterpart of the JSON configuration |
There was a problem hiding this comment.
That would not be safe, JSON Schema is loosy and defines only the structure.
1- It doesn't specify the size (Integer widths: uint8, 16, ...)
For an ASIL / safety-relevant config, silently inferring a wrong width (e.g. int64 where the C++ side expects uint16) is exactly the kind of defect that wouldn't be caught by flatc.
2- Enum sentinels e.g. INVALID = 0 is a deliberate "unset detection" (design mirroring) for QualityType
3- = null optional-scalar vs baked default
4- Table names for nested objects (e.g. ServiceTypeEvent vs InstanceEvent)
5- json_to_flatbuffer.py hardcodes the same key->field mapping
So yes, maintaining 2 Schemas is not great, but it's the safest way here (with existing one common testing for both - to come in next stories)
Also, JSON Schema is not changing that often, mostly stable, right?
What I suggest, is to implement a "drift-detection test" that parses the JSON schema and the .fbs and fails the build if their structure diverges.
This catches if someone edits the JSON schema and forgets the .fbs.
What do you think?
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
| """Convert an mw::com configuration JSON file into a FlatBuffer binary. |
There was a problem hiding this comment.
This converter tool is only a facilitating option for whom already has a JSON config and wants to use flatbuffer config binaries for its benefits.
It's converting to flatbuffer convertable json format (snake case)
User has the freedom to use any other method to get flatbuffer binaries directly.
For example:
- Programmatic builder (FlatBufferBuilder in C++, the flatbuffers package in Python, etc.) with any other text formats: YAML, TOML, XML, CSV, .ini.
- A database or network service — query rows/records and build directly.
- In-memory C++ objects
| //score/mw/com/impl/configuration/converter:json_to_flatbuffer | ||
| //score/mw/com/impl/configuration/converter:mw_com_config.bin |
There was a problem hiding this comment.
Is it okay if I remove them from here and add them to EXCLUDED_PUBLIC_TARGETS in quality/visibility_guard/parser.py?
|
|
||
| # Enables FlatBuffers-based mw::com configuration tooling (disabled by default, | ||
| # see score/mw/com/impl/configuration/flatbuffers_flags.bzl). | ||
| common:flatbuffers --//score/mw/com/impl/configuration:enable_flatbuffers=true |
| the pinned ``@flatbuffers//:flatc`` compiler, so ``.bin`` artifacts become part | ||
| of the build graph and can be consumed by other targets (e.g. as ``data``). | ||
| """ | ||
|
|
There was a problem hiding this comment.
I'll add it as
visibility(["//score/mw/com/impl/configuration/..."])
2510ac0 to
9bf11de
Compare
There was a problem hiding this comment.
Pull request overview
Introduces a FlatBuffers-based configuration schema for mw::com, along with Bazel build plumbing and Python tooling to (1) generate a rich JSON schema from the .fbs source of truth and (2) convert existing JSON configs into FlatBuffer binaries via flatc, guarded by a schema drift + round-trip test.
Changes:
- Adds
mw_com_config.fbsas the single source of truth for configuration structure, and checks in its generated JSON schema. - Adds Python tooling to generate the JSON schema (
generate_schema.py) and convert JSON configs to FlatBuffers (converter/json_to_flatbuffer.py), plus a drift/round-trip test. - Adds a Bazel feature flag/config (
--config=flatbuffers) to gate compilation of FlatBuffers-related targets, and wires in the FlatBuffers module dependency.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| score/mw/com/impl/configuration/schema_drift_test.py | Adds drift and round-trip tests validating schema generation and JSON↔FlatBuffers fidelity. |
| score/mw/com/impl/configuration/mw_com_config.fbs | Defines the FlatBuffers schema as the canonical config definition. |
| score/mw/com/impl/configuration/mw_com_config_schema.json | Updates the checked-in generated JSON schema output. |
| score/mw/com/impl/configuration/generate_schema.py | Adds generator that post-processes flatc --jsonschema into the rich schema. |
| score/mw/com/impl/configuration/flatbuffers_flags.bzl | Adds Bazel bool flag + config_setting to gate the FlatBuffers path. |
| score/mw/com/impl/configuration/converter/json_to_flatbuffer.py | Adds JSON→FlatBuffer converter with generic hyphen/underscore normalization. |
| score/mw/com/impl/configuration/converter/json_to_flatbuffer_rule.bzl | Adds Bazel rule wrapper (genrule) to produce .bin from JSON configs. |
| score/mw/com/impl/configuration/converter/BUILD | Adds Bazel targets for converter tool and example binary generation. |
| score/mw/com/impl/configuration/BUILD | Wires in FlatBuffers library, schema generator, and drift/round-trip test. |
| MODULE.bazel | Adds FlatBuffers Bazel module dependency. |
| .bazelrc | Adds build:flatbuffers config to enable the FlatBuffers configuration path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "mw_com_config_schema.json is out of sync with mw_com_config.fbs. " | ||
| "Re-run: bazel run " | ||
| "//score/mw/com/impl/configuration:generate_schema and commit the result.", |
| cmd = " ".join([ | ||
| "$(location %s)" % converter, | ||
| "--fbs $(location %s)" % fbs, | ||
| "--json $(location %s)" % json, | ||
| "--output $@", | ||
| "--flatc $(location %s)" % flatc, | ||
| ]), |
| def convert(fbs_path, json_path, output_path, flatc="flatc"): | ||
| """Convert ``json_path`` to a FlatBuffer binary at ``output_path``.""" | ||
| with open(json_path, encoding="utf-8") as handle: | ||
| config = json.load(handle) | ||
|
|
||
| normalized = _normalize(config, _enum_symbols(flatc, fbs_path)) | ||
|
|
| /// @default: 0 | ||
| /// @min: 0 | ||
| /// @max: 255 | ||
| /// std::uint8_t optional parameter, which describes, how many slots are assigned to IPC Tracing. A value of 0 disables tracing. Default is 0. If it is set to 0 and a trace-filter-config demands this field being traced, a WARN message will be logged. A value greater than zeroe for a proxy will just indicate the intent for the service element to be traced. A value greater than zero on the sceleton side will set the number of tracing slots. |
|
@castler |
4ed7a6c to
ece281a
Compare
|
@castler It's ready. can you please re-review? |
| //score/mw/com/impl/configuration/converter:json_to_flatbuffer_rule.bzl | ||
| //score/mw/com/impl/configuration:flatbuffers_flags.bzl |
There was a problem hiding this comment.
Why should these two files be public? I think you are missing a visibility within them.
There was a problem hiding this comment.
I think json_to_flatbuffer_rule.bzl should be public (it's a build rule meant to be loaded by any test/config package)
There was a problem hiding this comment.
After reading your comment about moving that to baselibs:
"can we initially push it here -> trigger that request to baselibs developers -> replace it here once they provide it?"
| normalization) plus ``flatc --binary`` against ``mw_com_config.fbs``. | ||
| """ | ||
|
|
||
| def json_to_flatbuffer( |
There was a problem hiding this comment.
Should we not have a generic rule for this in baselibs, and then have here one that just uses the mw_com_config.fbs?
There was a problem hiding this comment.
If I understand you correctly, then someone should add this in "baselibs" and with the their coming release this should be available from their package, correct?
If so, can we initially push it here -> trigger that request to baselibs developers -> replace it here once they provide it?
| The public JSON format uses hyphenated keys (e.g. ``asil-level``) and hyphenated enum | ||
| values (e.g. ``file-permissions-on-empty``), but FlatBuffers identifiers cannot contain | ||
| hyphens, so ``mw_com_config.fbs`` spells them with underscores. This script is a thin, | ||
| *generic* preprocessor: it rewrites ``-`` -> ``_`` in object keys and in enum-valued |
There was a problem hiding this comment.
Great that it is now generic - I think we should provide it from baselibs as this will be required by the other modules as well.
There was a problem hiding this comment.
"can we initially push it here -> trigger that request to baselibs developers -> replace it here once they provide it?"
There was a problem hiding this comment.
I do not fully understand. There is right now quite some infrastructure setup in Baselibs for flatbuffers:
https://github.com/eclipse-score/baselibs/tree/main/score/flatbuffers
IMHO its just wrong to put these generic scripts into mw/com.
What do we gain by it? For me its fine if we first place them here and then move them...but...it sounds more like double work to be honest. @LittleHuba should decide IMHO.
In any case those targets should not be public for now - to avoid usage. Esp. if we want to move them again.
There was a problem hiding this comment.
I agree with you, but I just need them currently for my coming PR (Flatbuffer Parsing Strategy) because my plan to verify by making the testing of JSON parser common for both with that extra step for the Flatbuffer is to convert the test's json excerpt into flatbuffers binary.
And delaying that until flatbuffers repo push that and release, this will take much of time.
@LittleHuba what do you think?
There was a problem hiding this comment.
The json_to_flatbuffer.py script is mw::com specific and is not required in baselibs.
The rationale for the script is to keep the user facing json schema identical when introducing the additional flatbuffers support, step by step.
The existing schema is not flatc compatible.
Long term it can be decided if the script is kept in order to avoid a breaking changes for the user configurations schema or if the schema is adapted to also meet flatc requirements (e.g. no '-' in keys as the name will be used to generate a read and builder method).
This was also briefly discussed in the initial demo PR / Issue.
#110 (comment)
The actual serialization can be done with the translated json and serialize_buffer rule or recommended serialize_versioned_buffer rule. (examples)
The json_to_flatbuffer_rule.bzl and other py scripts seem to use direct invocation of flatc. This should definitely be avoided.
All flatc interaction shall be wrapped via baselibs.
Json schema generation is available via generate_json_schema. If additional flatc features are required they should be brought to baselibs e.g. schema evolution check, this is currently not provided as a bazel rule. (I've not checked if this is needed for schema_drift_test.py)
There was a problem hiding this comment.
@limdor
Recursively rewrite ``-`` -> ``_`` in object keys and enum-valued strings.
When starting with a flatbuffer schema, using "_" instead of "-" will already be enforced by flatc as those names will be used for code generation.
It is a choice that needs to be done by mw::com developers if they want to keep this conversion (which is a reasonable thing to do for the user), or dropping it and change to a flatc compatible json format without "-" (breaking change in terms of configuration input files).
In baselibs flatbuffers i would discourage such a conversion feature as it is not required for the intended usage of flatbuffers (use keys and enums with "_" instead of "-").
Other changes in this PR such as generate_schema.py are generic and extend the flatc functionality.
There was a problem hiding this comment.
@castler I'll create a PR in baselibs and push the flatbuffers changes that was agreed with @OliverHeilwagen in it.
Then migrate the code to use the baselibs/flatbuffers and adjust the scripts to keep only the mw::com relevant code + schema_drift_test
I'll need just to drop the RoundTrip test in schema_drift_test.py; it is not of critical importance anyway, the tests of the new coming flatbuffer parser class will cover the integrity of the same config to be exactly read in both strategies
There was a problem hiding this comment.
The first question here seems to be:
Do we want to make this change in our configuration file visible to the user?
If no, we have two options:
- Pollute score_baselibs with migration paths that for the moment we only need in score_communication.
- Add custom logic in score_communication to take care of this piece of the conversion.
If yes, we must provide a good migration path for our users.
The basic rule we should follow:
Only decentralize what is specific to our codebase.
I intentionally did not go deep in the source code. I want to focus on a high-level user perspective here.
A user will likely want a bazel target to get the flatbuffer config from a json config. The rule behind this target could be defined in score_baselibs or in score_communication.
Bazel rules can use subrules, which means we can separate the steps of the conversion.
Having the key fixing in score_communication while having the conversion itself in score_baselibs is feasible.
Breaking the public API has no real benefit at this moment since this is a pure mechanical change with no improvement for the user.
Thus, IMO we should have the core logic of the conversion in score_baselibs (for reuse by others) and only our specific key bending in score_communication. The Bazel rule that we provide to the user would then be a combination of both steps.
If we end up finding a second case where keys must also be bent in configs of another module, we should shift this logic to score_baselibs directly. At that moment it would not be specific to score_communication anymore but a migration pattern for multiple modules.
There was a problem hiding this comment.
Sounds good, the core logic is available in baselibs from my point of view:
- serialize_buffer or serialize_versioned_buffer
- generate_cpp
- generate_json_schema
A user from com should be able to invoke something like:
# bazel rule defined by com
generate_com_config(
name = "demo_config",
data = "demo_config.json",
output = "demo_config.bin",
)
The com specifc rule can then invoke:
- json_to_flatbuffer.py for json format compatibility.
- serialize_buffer or serialize_versioned_buffer rule from baselibs with hardcoded com fbs schema (flatc --binary).
The schema generation support of generate_schema.py can be brought to baselibs, as it is a generic extension of the flatc --jsonschema feature.
There was a problem hiding this comment.
I created the PR
eclipse-score/baselibs#474
|
|
||
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") | ||
|
|
||
| def flatbuffers_flags(name = "flatbuffers_flags"): |
There was a problem hiding this comment.
Why do we provide a name attribute, if we do not use it?
There was a problem hiding this comment.
Buildifier warning
_"The macro "flatbuffers_flags" should have a keyword argument called "name".
It is considered a macro because it calls a rule or another macro "native.config_setting" on line 33.
By convention, every public macro needs a "name" argument (even if it doesn't use it).
This is important for tooling and automation."_
There was a problem hiding this comment.
should we then not just use it?
There was a problem hiding this comment.
We do not use it, this is just to satisfy the rule
_ = name # buildifier: disable=unused-variable
…he JSON schema from it. JSON schema attributes are ordered to be consistent with each schema generation Added Schema generating script: fbs -> json Added config converter json -> fbs Added Schema drift test Introduce "flatbuffers_enabled" experimental flag
…ng the conversion rule to bazel + Remove hardcoded "serviceVersion" schema keys + generate_schema.py can rebuild the json schema shared defs (based on fbs comment token) + json_to_flatbuffer_rule now contains the calling of flatc + json_to_flatbuffer_lib (instead of exporting py files)
a3e8606 to
44718eb
Compare
Uh oh!
There was an error while loading. Please reload this page.