-
Notifications
You must be signed in to change notification settings - Fork 53
Change the 4C CLI parser to CLI11 #1490
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
base: main
Are you sure you want to change the base?
Conversation
sebproell
left a comment
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.
Thanks @maxiludwig! I left a first review. In summary, the code requires some restructuring to facilitate testing.
| // Custom CLI11 formatter to add extra spacing between options | ||
| class SpacedFormatter : public CLI::Formatter |
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.
Do we need this? I guess this is just for the help message. If there are no serious flaws with the defaults (and I doubt there are any), I would just use what CLI11 provides.
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.
POSITIONALS:
input TEXT Name of the input file, including the suffix
output TEXT Prefix of your output files.
io_pairs TEXT ... More pairs of simulation <input> and <output> names. Only
necessary when using nested parallelism with multiple groups and
separate input files.
I found this a bot too dense, so i thought having the blank lines makes reading of the options easier.
POSITIONALS:
input TEXT Name of the input file, including the suffix
output TEXT Prefix of your output files.
io_pairs TEXT ... More pairs of simulation <input> and <output> names. Only
necessary when using nested parallelism with multiple groups and
separate input files.
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.
I think the other version is also fine but I agree that the spaced version is slightly more readable. By the way, could you post the full help message in the description of the PR?
bb08089 to
ff9394e
Compare
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.
Pull Request Overview
This pull request modernizes the command-line argument parsing in 4C by replacing legacy custom parsing logic with the CLI11 library. The changes introduce more robust argument validation, better error messages, and support for both legacy (single-dash) and modern (double-dash) option formats with deprecation warnings.
Key changes:
- Integration of CLI11 library as a required dependency for modern command-line parsing
- Introduction of helper functions to adapt legacy command-line arguments (single-dash options like
-ngroupand dashless options likerestart=) to CLI11-compatible format with deprecation warnings - Refactoring of I/O identifier logic into a centralized
update_io_identifiersfunction
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| cmake/configure/configure_CLI11.cmake | Configures CLI11 as a fetchcontent dependency (version 2.6.1) |
| CMakeLists.txt | Adds CLI11 as a required dependency |
| cmake/setup_install.cmake | Adds CLI11 to the list of dependencies in the config file |
| cmake/templates/4CConfig.cmake.in | Adds CLI11 package finding to the config template |
| src/core/comm/src/4C_comm_utils.hpp | Adds CommConfig struct and updates function signature to accept config pointer instead of argv vector |
| src/core/comm/src/4C_comm_utils.cpp | Refactors create_comm to use CommConfig struct instead of parsing argv internally |
| src/core/comm/tests/4C_comm_utils_test.np3.cpp | Updates test to use new CommConfig-based API |
| src/core/io/src/4C_io_command_line_helpers.hpp | Defines CommandlineArguments struct and helper functions for argument parsing and validation |
| src/core/io/src/4C_io_command_line_helpers.cpp | Implements legacy CLI adaptation, I/O pair building, cross-compatibility validation, and I/O identifier updates |
| src/core/io/tests/4C_io_command_line_helpers_test.cpp | Adds comprehensive unit tests for command-line helper functions |
| apps/global_full/4C_global_full_main.cpp | Replaces custom help text and parsing with CLI11-based parse_command_line function |
| apps/global_full/4C_global_full_io.hpp | Updates function signatures to accept CommandlineArguments struct and separate communicators |
| apps/global_full/4C_global_full_io.cpp | Removes legacy parsing functions and updates setup functions to use new argument structure |
| unittests/mat/4C_electrode_test.cpp | Updates to use new CommConfig-based communicator creation API |
| tests/input_files/*.4C.yaml | Updates documentation in test input files to show double-dash option format |
| doc/documentation/src/**/*.rst | Updates documentation examples to use double-dash option format |
| cmake/functions/four_c_testing_functions.cmake | Updates test command generation to use double-dash format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ff9394e to
47cccba
Compare
sebproell
left a comment
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.
@maxiludwig I think this is a nice improvement once you address the comments. Especially the nested parallelism looks like it could need more rework, but I like the increment and think we can merge.
| int ngroup = 1; | ||
| std::vector<int> group_layout; | ||
| NestedParallelismType np_type; | ||
| int diffgroup = -1; |
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.
Nice, the struct is much easier to understand than the old vector of strings 👍 Please document all these fields and their interaction here. Isn't n_group just group_layout.size()? If yes, drop it.
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.
When everything is setup properly, this needs to be the case.
|
|
||
| //! create a local and a global communicator for the problem | ||
| Communicators create_comm(std::vector<std::string> argv); | ||
| Communicators create_comm(CommConfig& config); |
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.
| Communicators create_comm(CommConfig& config); | |
| Communicators create_comm(const CommConfig& config); |
| /** | ||
| * \brief Structure to hold command line arguments. | ||
| */ | ||
| struct CommandlineArguments |
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.
This is placed wrongly now. The concrete parameters are a choice of the 4C main executable. Think about it this way: other executables should be able to use general CLI utilities that are in core, but of course they should not add a struct listing their parameters. Please move the struct back to the apps. There are a few functions that perform work on the struct, so they likely also belong to apps. Only the logical manipulations like the single-dash/no-dash fixes can be part of core. And any other helpers that we like to interface with CLI11. Currently, you use CLI11 directly in apps and I think that is fine. If we ever build abstractions on top of CLI11, they would again go in core. I am writing this to hopefully paint a clearer picture of how to sort functionality into the correct modules. I am not saying you should implement any kind of general cli interface right now.
| // Custom CLI11 formatter to add extra spacing between options | ||
| class SpacedFormatter : public CLI::Formatter |
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.
I think the other version is also fine but I agree that the spaced version is slightly more readable. By the way, could you post the full help message in the description of the PR?
| "Specify nested parallelism type: " | ||
| "separateInputFiles|everyGroupReadInputFile|\nnestedMultiscale|diffgroup<N> \n" | ||
| "Must be set if --ngroup > 1. \n" | ||
| "'diffgroupx' can be used to compare results from separate but parallel 4C runs; " |
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.
| "'diffgroupx' can be used to compare results from separate but parallel 4C runs; " | |
| "'diffgroupx' can be used to compare vectors/matrices/results between two separate (serial/parallel) 4C runs; " |
This pull request modernizes and streamlines the command-line argument parsing and I/O handling in the 4C
global_fullapplication. The main improvements are the introduction of the CLI11 library for argument parsing, the removal of legacy parsing logic, and the refactoring of input/output handling to support more robust and flexible usage, especially for nested parallelism.Key changes include:
4C_global_full_io.cppto adapt old legacy style command line arguments to make them parsable by CLI11. The sigle dashed options (-ngroup,-glayout, etc) and the restart arguments (restart=andrestartfrom=) are internally converted to double dashed option style--option. Warnings are thrown if the old option style is used.4C_global_full_main.cpp. Some checks doing cross argument validation are done right after that.CommandlineArgumentsstruct to use the already parsed arguments. Downstream functionalities (like4C_comm_utils.cpp) are no longer doing the actual parsing.update_io_identifiersfunction, improving clarity and reducing code duplication.