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

#2175: Add YAML as input as alternative to command-line arguments #2230

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from

Conversation

cwschilly
Copy link
Contributor

@cwschilly cwschilly commented Dec 20, 2023

Fixes #2175

Usage

Pass a yaml config file with

--vt_input_config_yaml /path/to/yaml/file

The order of precedence is currently

  1. input yaml
  2. command line arguments
  3. input toml or ini 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 to YAML::BaseNull (to avoid shadowing YAML::NodeType::Null). I don't foresee this having consequences for our development (if we want to check if some input is null, we would use YAML::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).

@cwschilly cwschilly self-assigned this Dec 20, 2023
@cwschilly cwschilly linked an issue Dec 20, 2023 that may be closed by this pull request
1 task
@cwschilly cwschilly marked this pull request as draft December 20, 2023 16:35
Copy link

github-actions bot commented Dec 20, 2023

Pipelines results

PR tests (gcc-12, ubuntu, mpich)

Build for 1863492 (2024-04-18 23:12:29 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-10, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (gcc-9, ubuntu, mpich, zoltan, json schema test)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-9, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-11, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-12, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-13, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-14, ubuntu, mpich)

Build for 1863492 (2024-04-18 23:12:29 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (intel icpc, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhibited by limit max-total-size 
remark #11076: To get full report use -qopt-report=4 -qopt-report-phase ipo
remark #11074: Inlining inhibited by limit max-size 
remark #11074: Inlining inhi%0D%0A%0D%0A%0D%0A ==> And there is more. Read log. <==

Build log


PR tests (gcc-11, ubuntu, mpich, trace runtime, coverage)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (nvidia cuda 11.2, gcc-9, ubuntu, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

/vt/src/vt/pipe/pipe_manager.impl.h(135): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>]"
          detected during:
            instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>]" 
/vt/src/vt/objgroup/proxy/proxy_objgroup.impl.h(204): here
            instantiation of "vt::objgroup::proxy::Proxy<ObjT>::PendingSendType vt::objgroup::proxy::Proxy<ObjT>::reduce<f,Op,Target,Args...>(Target, Args &&...) const [with ObjT=vt::vrt::collection::lb::GreedyLB, f=&vt::vrt::collection::lb::GreedyLB::collectHandler, Op=vt::collective::PlusOp, Target=vt::objgroup::proxy::ProxyElm<vt::vrt::collection::lb::GreedyLB>, Args=<vt::vrt::collection::lb::GreedyPayload>]" 
/vt/src/vt/vrt/collection/balance/greedylb/greedylb.cc(222): here

/vt/src/vt/pipe/pipe_manager.impl.h(135): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]" 
/vt/examples/callback/callback.cc(147): here

/vt/src/vt/pipe/pipe_manager.impl.h(135): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]" 
/vt/examples/callback/callback.cc(153): here

/vt/src/vt/pipe/pipe_manager.impl.h(135): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&MyObj::handler, Target=vt::objgroup::proxy::ProxyElm<MyObj>]" 
/vt/examples/callback/callback.cc(147): here

/vt/src/vt/pipe/pipe_manager.impl.h(135): warning: missing return statement at end of non-void function "vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]"
          detected during instantiation of "auto vt::pipe::PipeManager::makeSend<f,Target>(Target) [with f=&colHan, Target=vt::vrt::collection::VrtElmProxy<MyCol, vt::Index1D>]" 
/vt/examples/callback/callback.cc(153%0D%0A%0D%0A%0D%0A ==> And there is more. Read log. <==

Build log


PR tests (clang-13, alpine, mpich)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (intel icpx, ubuntu, mpich)

Build for 1863492 (2024-04-18 23:12:29 UTC)

In file included from /vt/lib/yaml-cpp/src/convert.cpp:3:
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:103:7: warning: comparison with NaN always evaluates to false in fast floating point modes [-Wtautological-constant-compare]
  if (std::isnan(rhs)) {
      ^~~~~~~~~~~~~~~
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:212:1: note: in instantiation of function template specialization 'YAML::conversion::inner_encode<float>' requested here
YAML_DEFINE_CONVERT_STREAMABLE_SIGNED(float);
^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:194:3: note: expanded from macro 'YAML_DEFINE_CONVERT_STREAMABLE_SIGNED'
  YAML_DEFINE_CONVERT_STREAMABLE(type, -)
  ^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:155:19: note: expanded from macro 'YAML_DEFINE_CONVERT_STREAMABLE'
      conversion::inner_encode(rhs, stream);                               \n                  ^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:105:14: warning: comparison with infinity always evaluates to false in fast floating point modes [-Wtautological-constant-compare]
  } else if (std::isinf(rhs)) {
             ^~~~~~~~~~~~~~~
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:103:7: warning: comparison with NaN always evaluates to false in fast floating point modes [-Wtautological-constant-compare]
  if (std::isnan(rhs)) {
      ^~~~~~~~~~~~~~~
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:213:1: note: in instantiation of function template specialization 'YAML::conversion::inner_encode<double>' requested here
YAML_DEFINE_CONVERT_STREAMABLE_SIGNED(double);
^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:194:3: note: expanded from macro 'YAML_DEFINE_CONVERT_STREAMABLE_SIGNED'
  YAML_DEFINE_CONVERT_STREAMABLE(type, -)
  ^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:155:19: note: expanded from macro 'YAML_DEFINE_CONVERT_STREAMABLE'
      conversion::inner_encode(rhs, stream);                               \n                  ^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:105:14: warning: comparison with infinity always evaluates to false in fast floating point modes [-Wtautological-constant-compare]
  } else if (std::isinf(rhs)) {
             ^~~~~~~~~~~~~~~
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:103:7: warning: comparison with NaN always evaluates to false in fast floating point modes [-Wtautological-constant-compare]
  if (std::isnan(rhs)) {
      ^~~~~~~~~~~~~~~
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:214:1: note: in instantiation of function template specialization 'YAML::conversion::inner_encode<long double>' requested here
YAML_DEFINE_CONVERT_STREAMABLE_SIGNED(long double);
^
/vt/lib/yaml-cpp/include/yaml-cpp/node/convert.h:194:3: note: expanded from macro 'YAML_DEFINE_CONVERT_STREAMABLE_SIGNED'
  YAML_DEFINE_CON%0D%0A%0D%0A%0D%0A ==> And there is more. Read log. <==

Build log


PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich)

Build for 1863492 (2024-04-18 23:12:29 UTC)

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "double" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, double> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "int" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, int> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Testing - passed

Build log


PR tests (gcc-12, ubuntu, mpich, verbose)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (intel icpx, ubuntu, mpich, verbose)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (clang-14, ubuntu, mpich, verbose)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

Compilation - successful

Testing - passed

Build log


PR tests (nvidia cuda 12.2.0, gcc-9, ubuntu, mpich, verbose)

Build for 8eda5a8 (2024-05-30 19:00:18 UTC)

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "double" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, double> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=double]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

/vt/lib/CLI/CLI/CLI11.hpp(1029): warning #2361-D: invalid narrowing conversion from "int" to "unsigned long"
          TT { std::declval<CC>() }
               ^
          detected during:
            instantiation of "vt::CLI::detail::is_direct_constructible<T, C>::test [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" based on template arguments <std::vector<std::string, std::allocator<std::string>>, int> at line 1041
            instantiation of class "vt::CLI::detail::is_direct_constructible<T, C> [with T=std::vector<std::string, std::allocator<std::string>>, C=int]" at line 5005
            instantiation of "void vt::CLI::Option::results(T &) const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 5034
            instantiation of "T vt::CLI::Option::as<T>() const [with T=std::vector<std::string, std::allocator<std::string>>]" at line 7315

/vt/tests/perf/send_cost.cc(169): warning #177-D: variable "prevNode" was declared but never referenced
    auto const prevNode = (thisNode - 1 + num_nodes_) % num_nodes_;
               ^

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

Testing - passed

Build log


@cwschilly cwschilly force-pushed the 2175-add-yaml-as-input-as-alternative branch from 4a97f5c to e2714fc Compare December 20, 2023 19:07
@cwschilly cwschilly marked this pull request as ready for review December 20, 2023 21:20
@cwschilly
Copy link
Contributor Author

I'm not sure what's going on with the PR checks (git --check) pipeline--there isn't any trailing white space in the file it's talking about

@JacobDomagala
Copy link
Contributor

JacobDomagala commented Dec 21, 2023

I think we should strip it down to bare minimum. We don't need test and contributing dirs. That would also resolve the trailing whitespace issue : )

Should we support using external yaml-cpp? Same way we intent to do with fmt (#2221)

@cwschilly
Copy link
Contributor Author

Should we support using external yaml-cpp? Same way we intent to do with fmt (#2221)

I think this makes sense, especially since it seems the yaml-cpp source code doesn't pass the CI

@cwschilly cwschilly marked this pull request as draft December 21, 2023 17:01
@JacobDomagala
Copy link
Contributor

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.

@lifflander
Copy link
Collaborator

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.

@cwschilly
Copy link
Contributor Author

cwschilly commented Jan 3, 2024

I've taken the approach of this PR (using enum classes) in order to resolve the clang warnings.

@cwschilly cwschilly marked this pull request as ready for review January 3, 2024 22:12
@cwschilly cwschilly force-pushed the 2175-add-yaml-as-input-as-alternative branch from 328fedf to 889748e Compare January 4, 2024 15:04
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;
Copy link
Contributor Author

@cwschilly cwschilly Jan 4, 2024

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.

@cwschilly cwschilly force-pushed the 2175-add-yaml-as-input-as-alternative branch 2 times, most recently from 0d5e7de to 18792a9 Compare January 5, 2024 13:43
@lifflander lifflander force-pushed the 2175-add-yaml-as-input-as-alternative branch from 3241ee7 to cf76e82 Compare April 18, 2024 21:49
@lifflander
Copy link
Collaborator

lifflander commented Apr 30, 2024

@cwschilly Summary of our discussion:

  • Let's output the current configuration as a YAML file
  • Use this output for testing input is working properly
  • For the debug print configuration, take a list of components to output debug print statements
  • Create further nesting for inputs, like the "LB Data Output"/"Statistics"/"LB Data Input"

@cwschilly cwschilly force-pushed the 2175-add-yaml-as-input-as-alternative branch from 8ba5c3b to c4cbec5 Compare May 21, 2024 16:28
@cwschilly cwschilly marked this pull request as ready for review May 24, 2024 19:02
@cwschilly cwschilly force-pushed the 2175-add-yaml-as-input-as-alternative branch from 451ba1c to e5b6e9f Compare May 28, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add YAML as input as alternative to command-line arguments
3 participants