Launch Manager supports the new configuration format#110
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
pawelrutkaq
left a comment
There was a problem hiding this comment.
The JSON files as test files are a bit bloat, maintaining them may be harsh. Maybe it would be better to have that in code and copose configs from well defined pieces.
nothing worrying, but next time it would be good to keep PR smaller.
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
| exports_files(["s-core_launch_manager.schema.json"]) |
There was a problem hiding this comment.
lets remove s-core from name. makse no sens imho. and it even wors with - and _ mixup
There was a problem hiding this comment.
How about we move the renaming to the other open PR regarding configuration schema then it can directly be changed consistently everywhere
|
|
||
| ControlClient::ControlClient(std::function<void(const score::lcm::ExecutionErrorEvent&)> undefinedStateCallback) noexcept { | ||
| ControlClient::ControlClient() noexcept { | ||
| static std::function<void(const score::lcm::ExecutionErrorEvent&)> undefinedStateCallback = [](const score::lcm::ExecutionErrorEvent& event) {}; |
There was a problem hiding this comment.
why static fn and not simply lambda ?
There was a problem hiding this comment.
I guess lambda is equally fine. I can change it to lambda if you prefer that.
The idea here was merely to hardcode this callback to an empty callback for now to align the interface with the existing documentation.
After further discussion on how the API should look like, we can then either delete this or refactor.
examples/run_examples.bzl
Outdated
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
| def _impl_run_examples(ctx): |
There was a problem hiding this comment.
why we just dont use sh_ or genrule ?
There was a problem hiding this comment.
Got rid of bzl file and replaced it with sh_binary
Regarding the JSON files. I agree it will not scale well. Currently, there are only few basic tests, because this configuration mapping is only temporary and the next step will be to read in the new config directly from the C++ code. I would expect we do some fine tuning of the new configuration and once we are happy we change the C++ code and the python script and its tests will all be obsolete. |
|
@eelcoem branch need to be up to date |
Done |
|
QNX x86 build fails due to download timeout, however the qnx arm build was successful.
|
Summary
This PR introduces a mapping of the new configuration schema to the existing configuration format. Therefore, allowing users to configure the launch_manager using the simpler configuration format. This approach is depicted here.
The configuration interface is a bazel function that validates the json configuration, maps the new configuration content to the existing configuration files that are in use by the C++ implementation. Note: This PR is not yet adapting the C++ code to load the new configuration directly, but instead it maps the new configuration to the old configuration format so the existing code keeps working as-is.
Usage Example from the reference_integration perspective:
Details
launch_manager_config, allowing users to easily make use of the new configurationlifecycle_config.py, taking as input the new configuration file and generating as output the same content in the existing configuration format (i.e. mapping of RunTarget -> ProcessGroup States). See the Readme at scripts/config_mapping/Readme.md for detailsCloses: #38