-
Notifications
You must be signed in to change notification settings - Fork 8
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
#2175: Add YAML as input as alternative to command-line arguments #2230
base: develop
Are you sure you want to change the base?
Conversation
Pipelines resultsPR tests (gcc-12, ubuntu, mpich) Build for 1863492 (2024-04-18 23:12:29 UTC)
PR tests (gcc-8, ubuntu, mpich, address sanitizer) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-10, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (gcc-9, ubuntu, mpich, zoltan, json schema test) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-9, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (gcc-10, ubuntu, openmpi, no LB) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-11, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-12, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-13, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-14, ubuntu, mpich) Build for 1863492 (2024-04-18 23:12:29 UTC)
PR tests (intel icpc, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (gcc-11, ubuntu, mpich, trace runtime, coverage) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (nvidia cuda 11.2, gcc-9, ubuntu, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-13, alpine, mpich) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (intel icpx, ubuntu, mpich) Build for 1863492 (2024-04-18 23:12:29 UTC)
PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich) Build for 1863492 (2024-04-18 23:12:29 UTC)
PR tests (gcc-12, ubuntu, mpich, verbose) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (intel icpx, ubuntu, mpich, verbose) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (clang-14, ubuntu, mpich, verbose) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich, verbose) Build for 8eda5a8 (2024-05-30 19:00:18 UTC)
|
4a97f5c
to
e2714fc
Compare
I'm not sure what's going on with the |
I think we should strip it down to bare minimum. We don't need Should we support using external |
I think this makes sense, especially since it seems the yaml-cpp source code doesn't pass the CI |
Also, is the use of a yaml-based config file incentivized by something (like Empire already using that file type)? If not, maybe we should consider using JSON for the config file as well? We already have it set up, and its structure is well-known to everyone. |
Yes, Empire is using YAML for their input and we want to be able to pass that directly to VT. LBAF and vt-tv are also using YAML so it seems that that route makes sense. |
I've taken the approach of this PR (using enum classes) in order to resolve the clang warnings. |
328fedf
to
889748e
Compare
YAML_CPP_API bool IsNull(const Node& node); // old API only | ||
YAML_CPP_API bool IsNullString(const std::string& str); | ||
|
||
extern YAML_CPP_API _Null BaseNull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the shadowing warning caused by this (previously named YAML::Null
) and YAML::NodeType::Null
, I've renamed this variable to YAML::BaseNull
. I don't imagine that this will have any far-reaching consequences, but I want to highlight it just in case.
0d5e7de
to
18792a9
Compare
3241ee7
to
cf76e82
Compare
@cwschilly Summary of our discussion:
|
…test for reading input yaml
8ba5c3b
to
c4cbec5
Compare
451ba1c
to
e5b6e9f
Compare
Fixes #2175
Usage
Pass a yaml config file with
The order of precedence is currently
toml
orini
file,meaning that the YAML will overwrite the command line arguments (which in turn overwrite the input
toml
file). I'm looking into ways to restore the priority of command line arguments over the config files.Changes to
yaml-cpp
I had to make changes to the yaml-cpp library in order to avoid variable shadowing errors. I mostly used the solution from this PR, which added
enum class
where appropriate.In one case, I had to change a variable name from
YAML::Null
toYAML::BaseNull
(to avoid shadowingYAML::NodeType::Null
). I don't foresee this having consequences for our development (if we want to check if some input is null, we would useYAML::NodeType::Null
).I also made
yaml-cpp
a system include directory to silence all warnings (-Wtautological-constant-compare
was throwing an error on the intel icpx pipeline--this issue explains why it is not a meaningful warning and can be safely silenced).