Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove requirement that suite definition file names begin with "suite_" #569

Merged
merged 6 commits into from
Jul 11, 2024
Merged

Remove requirement that suite definition file names begin with "suite_" #569

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3b65b1
Updates to remove requirements that suite files start with "suite_"
mkavulich May 2, 2024
adf3cc0
Update SUITE_DEFINITION_FILENAME_PATTERN regex
mkavulich May 20, 2024
552b1dc
Add logic to read an "alias" file (alias.json) that maps old suite na…
mkavulich May 20, 2024
f8fd26e
Backtracking out "suite aliasing", we are just going to force people …
mkavulich May 21, 2024
3a9c061
Move suite_blocked_data_suite.xml -> blocked_data_suite.xml, so the "…
mkavulich May 28, 2024
8821d64
Upgrade message about legacy naming to warning-level
mkavulich Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions scripts/ccpp_prebuild.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def parse_arguments():
verbose = args.verbose
debug = args.debug
if args.suites:
sdfs = ['suite_{0}.xml'.format(x) for x in args.suites.split(',')]
sdfs = ['{0}.xml'.format(x) for x in args.suites.split(',')]
else:
sdfs = None
builddir = args.builddir
Expand Down Expand Up @@ -181,8 +181,22 @@ def parse_suites(suites_dir, sdfs):
logging.info('Parsing suite definition files ...')
suites = []
for sdf in sdfs:
logging.info('Parsing suite definition file {0} ...'.format(os.path.join(suites_dir, sdf)))
suite = Suite(sdf_name=os.path.join(suites_dir, sdf))
sdf_file=os.path.join(suites_dir, sdf)
if not os.path.exists(sdf_file):
# 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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be logging.warn (or warning, whatever the correct syntax is)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I have made that addition here.

logging.info(f"Filename {os.path.basename(sdf_file_legacy)}")
logging.info(f"Suite name {sdf}")
sdf_file=sdf_file_legacy
else:
logging.critical(f"Suite definition file {sdf_file} not found.")
success = False
return (success, suites)

logging.info(f'Parsing suite definition file {sdf_file} ...')
suite = Suite(sdf_name=sdf_file)
success = suite.parse()
if not success:
logging.error('Parsing suite definition file {0} failed.'.format(sdf))
Expand Down
2 changes: 1 addition & 1 deletion scripts/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
CCPP_STATIC_SUBROUTINE_NAME = 'ccpp_physics_{stage}'

# Filename pattern for suite definition files
SUITE_DEFINITION_FILENAME_PATTERN = re.compile('^suite_(.*)\.xml$')
SUITE_DEFINITION_FILENAME_PATTERN = re.compile('^(.*)\.xml$')

# Maximum number of concurrent CCPP instances per MPI task
CCPP_NUM_INSTANCES = 200
Expand Down
15 changes: 10 additions & 5 deletions scripts/mkstatic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,11 +691,16 @@ def parse(self, make_call_tree=False):
suite_xml = tree.getroot()
self._name = suite_xml.get('name')
# Validate name of suite in XML tag against filename; could be moved to common.py
if not (os.path.basename(self._sdf_name) == 'suite_{}.xml'.format(self._name)):
logging.critical("Invalid suite name {0} in suite definition file {1}.".format(
self._name, self._sdf_name))
success = False
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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, should this be warning? Or leave it as debug if it throws a warning in the first place anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it invalid or just not found?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood what you were checking, I now think the message is okay. Thanks for the explanation.

self._name, self._sdf_name))
success = False
return success

# Check if suite name is too long
if len(self._name) > SUITE_NAME_MAX_CHARS:
Expand Down
0 ...blocked_data/suite_blocked_data_suite.xml → .../test_blocked_data/blocked_data_suite.xml
View file
Open in desktop
File renamed without changes.
Loading