Skip to content

Conversation

@bartgol
Copy link
Contributor

@bartgol bartgol commented Oct 8, 2025

Motivation

In EAMxx, we were forced to add custom YAML tags (like !!ints) to specify that a YAML sequence was meant to be a sequence of a particular type. The need comes from allowing strong type safety when dealing with empty lists, so that parsing my_double_vec: [] would indeed yield a param list storing a std::vector<double> of length 0. However, this added some clunkyness to our yaml file construction in EAMxx.

This PR changes approach: when parsing a YAML file and encountering an empty sequence, we store a EmptySeq object. In ParameterList, we then allow to call get<T>("my_seq") with any std::vector<S>: if the param list is non-const, we will replace the EmptySeq entry with a std::vector<S>, while if the param list is const we return a const ref to a static local var.

Testing

Added testing for this feature

Additional Information

There is a drawback in this approach: if the user accidentally does something like

ekat::ParameterList pl = ...;
auto& my_doubles = pl.get<std::vector<int>>("my_doubles");

and my_doubles was indeed still set to an instance of EmptySeq, it will replace it with a std::vector<int>, which was not intended. However, I consider this a minor corner case. Getting rid of custom-made YAML tags is probably outweighing the "risk".

@jeff-cohere
Copy link
Contributor

Hey Luca. I see this in the testing log:

Here's the content of /home/runner/_work/EKAT/EKAT/ctest-build/debug/Testing/Temporary/LastTestsFailed_20251008-1701.log:
110:yaml_parser

The LastTestsFailed log file from CTest only lists the test numbers/names that fail. Is the corresponding LastTests_xyz.log too long to include in the log file? I bet it probably is. :-)

@bartgol
Copy link
Contributor Author

bartgol commented Oct 8, 2025

Hey Luca. I see this in the testing log:

Here's the content of /home/runner/_work/EKAT/EKAT/ctest-build/debug/Testing/Temporary/LastTestsFailed_20251008-1701.log:
110:yaml_parser

The LastTestsFailed log file from CTest only lists the test numbers/names that fail. Is the corresponding LastTests_xyz.log too long to include in the log file? I bet it probably is. :-)

Yeah, I forgot to retest the last commit before pushing, a classic :)

The log is uploaded as an artifact in the following step, to keep the terminal output more "scrollable".

Copy link
Contributor

@jeff-cohere jeff-cohere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution, given the problem. I agree with your assessment of the risks.

Copy link
Member

@jgfouca jgfouca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Those ! tags were causing issues

@bartgol bartgol merged commit 6bde7e3 into master Oct 8, 2025
4 checks passed
@bartgol bartgol deleted the bartgol/parameter-list-empty-vector branch October 8, 2025 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants