Remove requirement that suite definition file names begin with "suite_" #569
Conversation
mkavulich
requested review from
climbfuji,
dustinswales,
gold2718,
grantfirl,
mwaxmonsky and
peverwhee
as code owners
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
This looks okay although I have a question about an error message still being correct.
| logging.debug(f"Filename {os.path.basename(self._sdf_name)}") | ||
| logging.debug(f"Suite name {format(self._name)}") | ||
| else: | ||
| logging.critical("Invalid suite name {0} in suite definition file {1}.".format( |
There was a problem hiding this comment.
Is it invalid or just not found?
There was a problem hiding this comment.
This check is for inconsistency between the SDF filename and the <suite name=> tag which actually defines the name of the suite (Suite._name). So when reading in a file suite_abcd.xml, the only two valid values for Suite._name would be suite_abcd or abcd. Any other value is invalid.
If you think this error message is too unclear I could update it to be more precise, since it's really about inconsistency and not necessarily "validity".
There was a problem hiding this comment.
Sorry, I misunderstood what you were checking, I now think the message is okay. Thanks for the explanation.
mkavulich
added
the
capgen-unification
…mes to new. Also fix logic that handles missing suite files.
…blocked data" test now pulls double-duty to confirm that new suite name convention works
climbfuji
left a comment
climbfuji
left a comment
There was a problem hiding this comment.
I think there are more suites that should be renamed in test_prebuild?
scripts/ccpp_prebuild.py
Outdated
| # If suite file not found, check old filename convention (suite_[suitename].xml) | ||
| sdf_file_legacy=os.path.join(suites_dir, f"suite_{sdf}") | ||
| if os.path.exists(sdf_file_legacy): | ||
| logging.info("Parsing suite definition file using legacy naming convention") |
There was a problem hiding this comment.
Should this be logging.warn (or warning, whatever the correct syntax is)?
There was a problem hiding this comment.
Good suggestion, I have made that addition here.
| return success | ||
| if not (os.path.basename(self._sdf_name) == '{}.xml'.format(self._name)): | ||
| if (os.path.basename(self._sdf_name) == 'suite_{}.xml'.format(self._name)): | ||
| logging.debug("Parsing suite using legacy naming convention") |
There was a problem hiding this comment.
Same here, should this be warning? Or leave it as debug if it throws a warning in the first place anyway?
There was a problem hiding this comment.
That was my thinking: I wanted a message here, but since it's already mentioned in a higher-priority message earlier I figured debug-level was appropriate here.
mkavulich
force-pushed
the
feature/suite_names_update
branch
2 times, most recently
from
9ad2c20 to
3a9c061
Compare
|
@climbfuji regarding the suite names, since we are still supporting the legacy naming convention I thought retaining a mix of old and new suite names in our unit tests was appropriate. |
3 participants
As part of a new set of suite naming guidelines (see NCAR/ccpp-doc#72), we are removing the requirement that suite definition file names begin with the literal string
suite_. Currently this requirement is hard-coded into CCPP prebuild (but not in capgen), so the changes in this PR are necessary to remove them.I included some back-compatibility logic to allow for the previous naming convention to work as well. So if a user attempts to use ccpp_prebuild.py with a suite named "suitename", it will first look for an SDF named "suitename.xml", and then (if not found) look for an SDF named "suite_suitename.xml".
User interface changes?: Yes, but not required; see above back-compatibility description.
Fixes: #568 Remove requirement that suite files begin with "suite_"
Testing:
test removed: None
unit tests: Renamed the SDF for test_blocked_data so that it tests the new suite naming conventions. Other tests were left alone, so the old naming conventions also get tested
system tests: ??
manual testing: Ran many different tests with SCM; see NCAR/ccpp-scm#477 for more details