Repository navigation
[Feat] Add CLI variable injection (-p/--parameter) and custom INPUT path (-in/--input) (PART I) - #8089
[Feat] Add CLI variable injection (-p/--parameter) and custom INPUT path (-in/--input) (PART I)#8089ZhouXY-PKU wants to merge 8 commits into
Conversation
Agent Governance Check
|
mohanchen
added
the
Compile & CICD & Docs & Dependencies
Critsium-xy
left a comment
Critsium-xy
left a comment
There was a problem hiding this comment.
Blocking issues: --check-input is broken (parse_command_line.cpp:59), .info path (driver.cpp:150), -p silently ignored (driver.cpp:125), and the non-MPI-safe error exit (main.cpp:107). Details inline.
| // Unreachable in normal flow: parse_args rejects unknown | ||
| // flags before this function runs. Kept as a defensive check | ||
| // so parse_command_line stays self-contained for tests. | ||
| const std::string msg = "Unknown option: " + arg; |
There was a problem hiding this comment.
--check-input reaches here: parse_args only sets check_mode and does not exit, so abacus --check-input now fails with "Unknown option: --check-input" and returns 1. The header comment ("never reach this function") is wrong. Please handle/skip it and add a test.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Restored --check-input handling, including both orders with -in. Standalone validation and combined parser tests pass. The existing MPI check-mode exit still produces a launcher error after successful validation; this was reproduced with the original merge-base CLI paths and is recorded separately from the repaired parsing behavior.
The required CPU checks passed; CI status is unavailable through the configured connector.
| // (3) write the input file | ||
| std::stringstream ss1; | ||
| ss1 << PARAM.globalv.global_out_dir << PARAM.globalv.global_in_card << ".info"; | ||
| ss1 << PARAM.globalv.global_out_dir << cli.input_file << ".info"; |
There was a problem hiding this comment.
Appending the user path to global_out_dir breaks for any path with a directory: -in ../cases/INPUT writes outside OUT.*, -in sub/INPUT fails silently (dir does not exist), -in /abs/INPUT gives OUT.x//abs/INPUT.info. Use the basename, or keep the fixed INPUT.info.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): The generated summary now uses only the INPUT basename inside OUT.. Real PW/LCAO runs passed with simple, subdirectory, parent-relative and absolute INPUT paths; expected .info files were written inside the output directory and no outside summaries were created.
The required CPU checks passed; CI status is unavailable through the configured connector.
| // explicitly; no global state is introduced (governance rule 1). | ||
| ModuleIO::ReadInput input(PARAM.globalv.myrank); | ||
| input.read_parameters(PARAM, PARAM.globalv.global_in_card); | ||
| // input.read_parameters(PARAM, cli.input_file, cli.vars); |
There was a problem hiding this comment.
cli.vars is parsed and validated but never used, so -p ecutwfc 120 is silently ignored. Until PART II lands, -p should error or at least warn. Also please drop the commented-out call.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): PART I now prints one explicit warning when -p variables are supplied, before reading INPUT, and the dead read_parameters call/comments were removed. Parsing and last-wins storage are preserved; negative values and one warning under two MPI ranks were verified. Variable substitution remains deferred to PART II.
The required CPU checks passed; CI status is unavailable through the configured connector.
| ModuleIO::ReadInput input(PARAM.globalv.myrank); | ||
| input.read_parameters(PARAM, PARAM.globalv.global_in_card); | ||
| // input.read_parameters(PARAM, cli.input_file, cli.vars); | ||
| input.read_parameters(PARAM, cli.input_file); |
There was a problem hiding this comment.
-in only redirects INPUT. STRU/KPT/pseudo/orbital paths still resolve against CWD, so -in cases/si/INPUT fails to find STRU or silently picks up ./STRU. PARAM.globalv.global_in_card also stays stale ("INPUT"). Please document the intended semantics or resolve relative to the INPUT dir.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Documented that -in selects only INPUT while relative STRU/KPT, pseudopotential, orbital, restart and output paths remain relative to the working directory. Custom-path runs with conflicting STRU/KPT files beside INPUT verified this behavior. The unused legacy global_in_card member remains deferred cleanup; current read/log/output paths use the explicit CLI filename.
The required CPU checks passed; CI status is unavailable through the configured connector.
| const std::string err = e.what(); | ||
| std::fprintf(stderr, "Command line error: %s\n", err.c_str()); | ||
| ModuleIO::print_help(argv[0]); | ||
| return 1; |
There was a problem hiding this comment.
MPI is already initialized here: return 1 skips Parallel_Global::finalize_mpi(), and every rank prints the error + help. Print on rank 0 only and exit through the existing quit path (e.g. WARNING_QUIT).
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Run-control validation now finishes before MPI and FFTW initialization, so malformed arguments return without acquiring those resources. Single-process and two-rank error probes passed without improper-MPI-termination diagnostics. The suggested WARNING_QUIT was not used because it exits before its finalization block; pre-initialization CLI diagnostics retain the existing per-process behavior.
The required CPU checks passed; CI status is unavailable through the configured connector.
| // the token count and skip the tokens, so they survive the | ||
| // unknown-argument fallback below. No state is stored here | ||
| // (governance rule 1: no new globals). | ||
| else if (arg == "-p" || arg == "--parameter") |
There was a problem hiding this comment.
This duplicates the token-count checks of parse_command_line with a different error path (exit(1) vs throw), and every flag must be registered in both parsers (which is how --check-input broke). Consider a single parser that returns CommandLineArgs (including check_mode) and is called once.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Both parsers now share run-control option arity and token-count validation. The early informational scan remains for compatibility; combined-flow tests cover --check-input/-in ordering, negative and flag-looking values, malformed arguments, and help precedence. These focused tests pass.
The required CPU checks passed; CI status is unavailable through the configured connector.
| os << " -i, -I, --info Display detailed build information\n"; | ||
| os << " -h, --help [param] Display help for parameter (or this message)\n"; | ||
| os << " -s, --search <query> Search for parameters matching query\n"; | ||
| os << " -p, --parameter <name> <value> (Not taking effects yet)\n"; |
There was a problem hiding this comment.
Inconsistent with ModuleIO::print_help, which says -p "overrides INPUT 'variable'" and is usable as ${name}. Keep one help text in one place, and remove the commented-out lines below.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): General help is now the single detailed usage text; the error helper only points to -h. Help, runtime warning, headers and quick-start documentation consistently describe PART I nonapplication, and commented future claims were removed. Help/search/YAML checks passed.
The required CPU checks passed; CI status is unavailable through the configured connector.
| @@ -0,0 +1,116 @@ | |||
| #include "source_io/parse_command_line.h" | |||
There was a problem hiding this comment.
AGENTS.md rule 11: new test files should be named test_<module>.cpp, i.e. test_parse_command_line.cpp (update CMake target/SOURCES accordingly).
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Renamed the new source to test_parse_command_line.cpp and updated CMake SOURCES, while retaining the existing CTest target name. The parser target was discovered and passed in both MPI-enabled and serial configurations.
The required CPU checks passed; CI status is unavailable through the configured connector.
| /// Builds an argv array from strings and returns the token count | ||
| /// (excluding the terminating nullptr) via the out-parameter. | ||
| /// Lifetime note: `storage` and `strings` must outlive the parse call. | ||
| int make_argv(std::vector<std::string>& strings, |
There was a problem hiding this comment.
Missing #include <vector> (currently relies on gtest's transitive include).
| AddTest( | ||
| TARGET MODULE_IO_parse_command_line_test | ||
| LIBS parameter base device io_input | ||
| SOURCES parse_command_line_test.cpp ../parse_command_line.cpp |
There was a problem hiding this comment.
parse_command_line.cpp only depends on the standard library; parameter base device io_input are unnecessary here.
There was a problem hiding this comment.
Addressed by d3165ed (verified at dff4d7e): Removed parameter/base/device/io_input from this pure parser test and moved its registration before the MPI-only return. Nonempty CTest selections passed: parser/parse_args/help 3/3 in the MPI configuration and the standalone parser 1/1 with ENABLE_MPI=OFF.
Checks: cmake --build build -j4 --target abacus_basic_para MODULE_IO_parse_args MODULE_IO_parse_command_line_test MODULE_IO_input_help_test; ctest --test-dir build -j1 --output-on-failure --no-tests=error -R '^MODULE_IO_(parse_args|parse_command_line_test|input_help_test)$'. The serial parser used the same CTest options with --test-dir build-serial -R '^MODULE_IO_parse_command_line_test$'.
The required CPU checks passed; CI status is unavailable through the configured connector.
|
@ABACUS-PR-BOT Please try to fix this PR according to the comments above. |
|
@ABACUS-PR-BOT Please try to resolve the conflicts. |
|
Resolved the conflicts with Validation passed on this commit: CPU MPI/LCAO build, 12 focused CTests, 2 serial tests, and nine normal PW/LCAO runs covering default/custom INPUT paths. CI remains unverified. |
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.Linked Issue
Part 1 of 2 of the CLI variable-injection feature. This PR implements the
command-line parsing layer and the
-in/--inputwiring; the actualsubstitution of
${var}/$varinside INPUT is delivered by PART II(
ReadInputsignature extension + token-level substitution), which dependson this PR. No dedicated issue exists for the split; this PR body serves as
the phase tracker.
Unit Tests and/or Case Tests for my changes
ParseCommandLineTestpass (short/long form,last-wins for repeated
-p,-incombination, default input path,missing-value/empty-name/digit-leading-name/unknown-option/truncated-argc
all throw).
test_parse_argspassthrough unchanged.--helpoutput now lists-p, --parameterand-in, --input(text addedto
show_general_help()so it flows through the existing-hpath).-in INPUT.alt: reads the given file,running_scf.logshowsglobal_in_card = INPUT.alt,OUT.AltRun/INPUT.alt.infowritten;default run (no
-in) byte-identical to pre-change behavior.behavior verified unchanged on 104_PW_NC_magnetic, and all changes are
gated behind new flags that default to prior behavior.
-pon INPUT values: not applicable in PART I(see "What's changed"); the parsed variables are validated and stored but
do not yet affect INPUT. Covered in PART II with its own tests
(
test_var_substitute, read-input token substitution, variable keyword).What's changed?
User-visible changes:
-p, --parameter <name> <value>(repeatable, last wins): parse-timevalidated (non-empty, non-digit-leading, alnum/underscore name), stored in
ModuleIO::CommandLineArgs::varsand forwarded toDriver::init().In PART I these values are not yet applied to INPUT parameters — that
is PART II (
ReadInputvariable substitution). Accepted and validated nowso the CLI contract is frozen early and PART II only touches the INPUT layer.
-in, --input <file>(defaultINPUT): fully functional. Overrides theINPUT file path for reading, the
global_in_cardline inrunning_*.log,and the
INPUT.infofilename in the output directory.--help/-htext extended accordingly (inshow_general_help(), thesingle source of help text;
parse_command_line's own usage string remainsminimal and is only shown on CLI error paths).
one-line message; existing flags (
-h,--version,--search,--check-input,--generate-parameters-yaml) behave exactly as before.Developer-facing changes:
source_io/parse_command_line.{h,cpp}(new):CommandLineArgs{vars, input_file}andparse_command_line(argc, argv).parse_argsforwards-p/-intokens to it (additive change; all pre-existing flags handledwhere they were before).
source_main/driver.{h,cpp}:Driver::init/atomic_world/readingtakeconst ModuleIO::CommandLineArgs&;reading()passescli.input_fileexplicitly to
read_parameters(PARAM, cli.input_file),print_start_info(input_card), and theINPUT.infopath;print_start_infogains a named
input_cardparameter. No globals added; everything flowsthrough explicit parameters (governance rule 1).
source_io/input_help.cpp: help text for the two new options only.Governance Notes
untouched. Docs PR to follow in PART II when
-ptakes effect(documenting
${var}/$varsyntax,variablekeyword, precedencecommand line > INPUT
variable> built-in default, and-in).CLI flags, extended
--help, and theglobal_in_cardlog line /INPUT.infofilename now reflecting-in's value (previously alwaysINPUT).Changes are confined to
source_io(parse/help) andsource_main(driver wiring).
read_parameterskeeps its two-parameter signature inthis PR; PART II will extend it and update its call sites
(
driver.cppand read-input tests) — no default parameters are used(governance rule 5).
System_para::global_in_cardis now effectively dead in the main flow:all three of its former consumers in
driver.cpp(read path, log line,INPUT.infopath) readcli.input_fileexplicitly instead. The memberis kept because (a) removing it is an unrelated refactor that would bloat
this feature diff, and (b) a full-repo audit is required first (para_json
serialization, tests, downstream forks). Planned follow-up: audit with
grep -rn "global_in_card" source/, then a standalone cleanup commitremoving the member and updating any stragglers.
parse_command_line's internal usage string intentionally duplicates no--helpcontent; on CLI errors it should print the minimal hint plusRun 'abacus -h' for usage.(kept minimal to avoid text drift withshow_general_help()).