Xios log file config bug#549
Conversation
…iles and collate them on success into log_dir
| mpi_parts_xios = {{task_values["mpi_parts_xios"]}} | ||
| XIOS_INFO_LEVEL = {{task_values['xios_info_level']}} | ||
| XIOS_PRINT_FILE = {{task_values['xios_print_file']}} | ||
| XIOS_PRINT_FILE = {{".TRUE." if task_values['xios_print_file'] else ".FALSE."}} |
There was a problem hiding this comment.
Since this is checking whether there is anything present for 'xios_print_file', I would also update the documentation to reflect this. Otherwise, it may lead to confusion when users try to set this to 'false' and it doesn't work.
There was a problem hiding this comment.
thanks Oakley Brunt (@oakleybrunt)
i agree, this would no longer be consistent
i've updated teh docs and defaults to use this as a Boolean and then this logic holds, i think
returned to you for re-consideration
There was a problem hiding this comment.
I think that this needs to be thought about more carefully. The truthy value of a dictionary key containing a string of length >0 is True.
I would suggest either updating the documentation to reflect the current logic (that any value for 'xios_print_file' will result in TRUE).
or
Updating the docs to instruct users to use a suitable format for their values (e.g. .true. and .false.) and undoing this change to generate runtime. Then the onus is on them to use correct syntax.
I would recommend the second option since the first is not consistent with the majority of task options. But, the choice is yours, both are fine with me.
Oakley Brunt (oakleybrunt)
left a comment
There was a problem hiding this comment.
Hi Mark, while this has good intentions, I think it needs a slightly different approach.
| {% if "xios_info_level" not in task_dict %} | ||
| {% do task_dict.update({"xios_info_level": 0}) %} | ||
| {% do task_dict.update({"xios_print_file": "false"}) %} | ||
| {% do task_dict.update({"xios_print_file": False}) %} |
There was a problem hiding this comment.
I am now worried that this will not work for the same reason stated before. If there is text present in 'xios_print_file' then it will be set to .TRUE. and "False" will count as text.
| - Boolean | ||
| - False | ||
| - Controls whether XIOS creates ``xios_<client/server>_<rank>.out`` and ``xios_<client/server>_<rank>.err`` files. When set to "False", logging is directed to stdout and stderr. If ``xios_info_level`` is greater than 1, ``xios_print_file`` will be set to "True". |
There was a problem hiding this comment.
These docs give the impression that you can set this task option to False but this is not true.
| mpi_parts_xios = {{task_values["mpi_parts_xios"]}} | ||
| XIOS_INFO_LEVEL = {{task_values['xios_info_level']}} | ||
| XIOS_PRINT_FILE = {{task_values['xios_print_file']}} | ||
| XIOS_PRINT_FILE = {{".TRUE." if task_values['xios_print_file'] else ".FALSE."}} |
There was a problem hiding this comment.
I think that this needs to be thought about more carefully. The truthy value of a dictionary key containing a string of length >0 is True.
I would suggest either updating the documentation to reflect the current logic (that any value for 'xios_print_file' will result in TRUE).
or
Updating the docs to instruct users to use a suitable format for their values (e.g. .true. and .false.) and undoing this change to generate runtime. Then the onus is on them to use correct syntax.
I would recommend the second option since the first is not consistent with the majority of task options. But, the choice is yours, both are fine with me.
PR Summary
Sci/Tech Reviewer: Oakley Brunt (@oakleybrunt)
Code Reviewer:
A bug was introduced by #397 and testing did not cover the case where the bug is triggered.
this was missed at review (by me) and needs fixing
the XIOS logical parameter
print_fileneeds to take a value which is a Fortran boolean, so:/true.TRUE.this PR includes the fix for the task dictionary translation into working syntax for the environment variable and enables this path for a set of large rank count jobs (C224 & C896) to ensure the code is included in testing
I also add log collation into the log_dir in the generate_runtime_application, needed for statistics analysis.
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - xios_log_file_config_bug/run1
Suite Information
Task Information
✅ succeeded tasks - 1202
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review