Add logging variables to the config file#121
Add logging variables to the config file#121ischeider wants to merge 1 commit into4C-multiphysics:mainfrom
Conversation
also add command line switches
| # rapidyaml may emit non-standard JSON tokens for special float values | ||
| # such as 'inf', '-inf' or 'nan' (sometimes lowercase). The standard | ||
| # json.loads() will raise JSONDecodeError for these. Replace those | ||
| # literal tokens with null so the JSON parser can load the document. | ||
| # We only replace standalone tokens that appear as values (followed by | ||
| # a comma, closing bracket or closing brace). | ||
| json_str = regex.sub( | ||
| r"(?<=:\s)(?:-?inf|nan)(?=[,\]\}])", "null", json_str, flags=regex.IGNORECASE | ||
| ) | ||
|
|
There was a problem hiding this comment.
This should not be necessary anymore with the last release of rapidyaml.
See biojppm/rapidyaml#574, biojppm/rapidyaml#535 and biojppm/rapidyaml#312
I suspect that you do not have the latest rapidyaml version installed? Otherwise also our pipeline should fail.
There was a problem hiding this comment.
FYI that was the old way we had it implemented #119
There was a problem hiding this comment.
I have ryml 0.10.0 installed, and I get the error without it. What should I do?
There was a problem hiding this comment.
Yes that was fixed in 0.11.0 ;)
There was a problem hiding this comment.
okay, but the requirements.txt in the root directory says 0.10.0.
Anyway, with 0.11, the error is indeed gone.
There was a problem hiding this comment.
@ischeider I am sorry, I forgot to update that file in the last PR. In the upstream repos we always compile this file new that's why it did not come up anywhere else.
You can just update the requirements.txt file with
pip-compile --all-extras --output-file=requirements.txt --upgrade requirements.in
There was a problem hiding this comment.
We should add rapidyaml>=0.11 in the requirements.in to ensure even in the case we don't do a safe install that it still works :)
| log_output_flag: false | ||
| log_output_path: null |
There was a problem hiding this comment.
I don't see why we should have this in the config. The config is related to the specific 4C input version, which, if I understand correctly, is not the case for these options.
| # Determine whether logging should be enabled. Precedence: | ||
| # 1. CLI --log-file (explicit path) | ||
| # 2. CLI --enable-log | ||
| # 3. CONFIG.log_output_flag | ||
| # When enabled, if a file path is provided (CLI or CONFIG.log_output_path) use it | ||
| # and open with mode='w' (replace any existing file). Otherwise log to stdout. | ||
| try: | ||
| log_file_arg = getattr(parsed_args, "log_file", None) |
There was a problem hiding this comment.
Tbh, this feels a bit overengineered 😅
IMO, logging is something case-specific and not part of the project config. A log-file path in the config does not make sense to me 🤔
What if we keep the arguments you've added, with the defaults that seem sensible to you (for example: default logging enabled, default file something like fourcipp.log in the current directory?) and drop the config part? I think you are the power user of this feature as well as a motivated contributor, so I think it's fair for you to decide the defaults :)
Is there a particular reason for you to want this in the config?
There was a problem hiding this comment.
Actually, the particular reason was the error due to the "old" ryml version. And I did not understand, why there is a logging capability which has to be switched on in the code by deleting the disable.
Anyway, I am not sure whether the proposed feature is really needed, so if you feel it's not, I am perfectly fine with closing the PR without merging.
There was a problem hiding this comment.
My 'beef' was only with the config part of the feature, if you enjoy having the flag in the CLI, I support it :) I'll leave it up to you :)
3 participants
Description
The 4Cipp code has a logging option, which is explicitly disabled in the code. I thought it is a good idea to have a flag in the config file or a command line switch to enable logging (to screen or to a file). This is what I propose in this PR.
Please tell me whether you think it's useful.
The problem I see is that the config file is part of the repo itself, and as such might be overwritten if there is a merge conflict (which could happen in this PR...)
The change in
yaml_io.pyis included because I got an error message when running 4Cipp, and the copilot told me this is caused by a misinterpretation of specific values for floats, which are now converted tonone.