Fix issue #2123: Ensure command-line arguments take precedence over config file options#2132
Conversation
…onfig file options - Store original command-line arguments before config file loading - Re-apply CLI args after config file to ensure final precedence - Add tests verifying command-line takes precedence over config file - Add tests verifying config file applies when no CLI override exists - Follows priority order: CLI > config file > class init > env vars
GernotMaier
force-pushed
the
fix/issue-2123-command-line-precedence
branch
from
d6e66f6 to
cc795b1
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes configuration precedence so that explicitly provided command-line arguments override conflicting values from a YAML configuration file, aligning Configurator behavior with standard CLI expectations.
Changes:
- Store the resolved CLI argument list in
Configuratorand re-parse it after loading a config file to enforce precedence. - Add unit tests validating CLI-over-config precedence and the “config-only” behavior.
- Add a towncrier changelog fragment documenting the user-visible change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/simtools/configuration/configurator.py |
Persist parsed CLI args and re-apply them after _fill_from_config_file() during initialize() to ensure CLI precedence. |
tests/unit_tests/configuration/test_configurator.py |
Add tests covering CLI-over-config precedence and config-only behavior. |
docs/changes/2132.feature.md |
Document the precedence fix in a changelog fragment. |
EshitaJoshi
left a comment
EshitaJoshi
left a comment
There was a problem hiding this comment.
I'm approving because this does seem like the simplest/smallest change to fix the issue, and the code is fine, but I don't like that self._fill_config is basically called multiple times in a row.
If we want to focus on maintainability and readability of code, I think we should think about using self.config.update() to make it clear what takes precedence over what, because here after the CLI arguments are re-applied it looks like they are rewritten again by the class variables (config_dict) and also the environment variables due to the order of when the functions are being called. But that's not true - the class variables and the env variables dont overwrite existing variables.
if we want to make that more readable, i would suggest having something like
self.config.update(env_variables)
self.config.update(class_variables)
self.config.update(file_variables)
self.config.update(cli_variables)
just so you know what overwrites what.
I guess that would be a larger refactor though, and this currently does fix the open issue with not that much more code, so I leave it up to you what you think is best.
|
@EshitaJoshi - thanks a lot. Very good point! I've tried to implement this, can you have another quick look? |
EshitaJoshi
left a comment
EshitaJoshi
left a comment
There was a problem hiding this comment.
Thanks for the PR! Minor comments.
Also, _fill_from_environmental_variables, _fill_from_config_file, and _fill_from_config_dict can be deleted since they have been replaced
| def _fill_from_command_line(self, arg_list=None, require_command_line=True): | ||
| """ | ||
| Fill configuration parameters from command line arguments. | ||
|
|
||
| Triggers a print of the help if no command line arguments are given and | ||
| require_command_line is set. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| arg_list: list | ||
| List of arguments. | ||
| require_command_line: bool | ||
| Require at least one command line argument. | ||
|
|
||
| """ | ||
| self._fill_config(self._get_cli_arglist(arg_list, require_command_line)) |
There was a problem hiding this comment.
This function is not being called anywhere and so can be removed
There was a problem hiding this comment.
Good point. Removed (needed some adjustments in the unit tests)
|
Thanks again for the review @EshitaJoshi - I've addressed all your comments. Can I merge (after the tests run through)? |
|
@GernotMaier I think you still have to delete |
You are as always right. I've removed this part. |
Allow to specifiy model version dependent configuration parameters in configuration files
|
Summary
Command-line arguments now correctly take precedence over configuration file options.
Problem
Previously, when both a configuration file and command-line arguments were provided, configuration file values would override command-line values. This violated the standard convention where CLI args should have higher priority.
Solution
Changes
Configurator.__init__()to track original command-line args_fill_from_command_line()to store args before parsing_reapply_command_line_args()method to re-apply CLI args after config fileinitialize()to call re-apply after config file loadingAllows now e.g.
simtools-simulate-prod --config config_file.yml --run_number 5(mix of config and command line values)Testing
configurator.py