Changes to filename_template checks

[abishekg7]

Issue:

Previously, if two active, output stream definitions in the streams. file specified the same string as the filename_template attribute, the corresponding MPAS CORE would crash with an error message. This was also the case for when one of the streams, sharing the filename_template, included an inactive package, hence effectively making it inactive. The existing logic to perform stream uniqueness checks in src/framework/xml_stream_parser.c did not have the information about which packages associated with a stream were active, and hence did not account for inactive packages.

Solution

This PR introduces a new subroutine MPAS_stream_mgr_check_filename_template in src/framework/mpas_stream_manager.F to check whether two active streams might potentially be accessing the same file (via filename_template) resulting in file conflicts. More specifically, this subroutine checks that there are no active output streams that share an identical filename_template attribute with any other active input or output streams in the stream manager. An active stream in this context is a stream with attribute active_stream = .true. and is not deactivated by an inactive package and is either an input or output stream with at least one active alarm. This routine can be called from within MPAS_stream_mgr_validate_streams or separately as needed.

This PR also introduces a new function 'MPAS_stream_mgr_get_num_alarms', which returns the number of active alarms for a given stream in the specified direction.

The previous logic to check for unique filename_template attributes in src/framework/xml_stream_parser.c has been removed, while retaining the checks for unique stream names in the same location. Note that the previous logic only check for conflicts between two output streams, whereas the new logic also checks for potential conflicts between an input stream and an output stream accessing the same file.

[mgduda]

I've run a few quick tests, and it looks like the core logic is now functioning as expected. I had just a couple of ideas that might clean up the implementation:

1. The outer loop (with stream1_cursor) could cycle on anything other than an active output stream, and that might obviate the need for checks like (stream1_input .and. stream2_input) in the inner loop.
2. If the inner loop (with stream2_cursor) iterated only over streams that occur in the stream list after stream1_cursor, that would eliminate duplicate error messages (e.g., "Found identical values ... for ... foobar_1 and foobar_2" and later "Found identical values ... for ... foobar_2 and foobar_1").

[abishekg7]

I've run a few quick tests, and it looks like the core logic is now functioning as expected. I had just a couple of ideas that might clean up the implementation:

1. The outer loop (with stream1_cursor) could cycle on anything other than an active output stream, and that might obviate the need for checks like (stream1_input .and. stream2_input) in the inner loop.
2. If the inner loop (with stream2_cursor) iterated only over streams that occur in the stream list after stream1_cursor, that would eliminate duplicate error messages (e.g., "Found identical values ... for ... foobar_1 and foobar_2" and later "Found identical values ... for ... foobar_2 and foobar_1").

I've incorporated the 2nd idea in the most recent commit. But having either the outer loop or the inner loop consider only output streams, appears to miss some filename conflicts, due to the directionality introduced by idea #2.

[mgduda]

Could we add a sentence or two about the return values from this routine? That may be especially relevant, since the return value of this routine may be used directly as the return value to MPAS_stream_mgr_validate_streams.

It might also be helpful to add a bit more detail as to what exactly this routine is checking. For example, the description "among active I/O streams" suggests that two active input streams might be caught by this routine.

[mgduda]

Could you take another pass through the code changes in this PR and regularize the whitespace? For example, there's no space between err_local and err_local1 here. Other places, the whitespace is inconsistent within parentheses (e.g., lines 608 and 612).

[mgduda]

It doesn't look like the err_local1 or err_local2 return error codes are ever used. It might simplify the code to make ierr an optional argument to MPAS_stream_mgr_get_num_alarms, and to not pass any corresponding actual argument in these calls. That would also allow the local variables err_local1 and err_local2 to be removed.

[mgduda]

I may be mistaken, but with the exception of stream1_input, the above stream1_* variables are only used until the point where we enter the second loop over streams, and so it might be possible to simply declare the variables pkg_active (rather than stream1_pkg_active and stream2_pkg_active), etc. This would cut down on the number of local variables that the reader would need to consider when looking through this code.

[mgduda]

The error message can be quite long when printed to a single line, e.g.,

ERROR: Found identical values of the filename_template attribute for multiple active output streams, restart and output, in streams.<CORE>. This may result in file conflicts.

It might be better to explicitly write a multi-line error message with multiple calls to mpas_log_write.

[abishekg7]

I have addressed the remaining suggestions in the last few commits.