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

Jump to bottom

Kdraeder/fix st archive #1916

Merged
merged 5 commits into from
Sep 28, 2017
Merged

Kdraeder/fix st archive #1916

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd87719
Committing all changes, may want to undo some.
kdraeder Sep 18, 2017
1c2446f
Updating scripts/lib/CIME/case_st_archive.py
kdraeder Sep 22, 2017
5eef510
resolved merge conflicts. Also responded to comments
kdraeder Sep 26, 2017
7ac36e3
Re-introducing changes based on reviews of first push. There were 2 …
kdraeder Sep 27, 2017
8ea228a
Removed outdated debugging statement and 2 lines of redundant code.
kdraeder Sep 28, 2017
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
1 change: 0 additions & 1 deletion scripts/lib/CIME/case_setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ def _case_setup_impl(case, caseroot, clean=False, test_mode=False, reset=False):
if comp == "CPL":
continue
ninst = case.get_value("NINST_{}".format(comp))
# But the NINST_LAYOUT may only be concurrent in multi_driver mode
if multi_driver:
expect(case.get_value("NINST_LAYOUT_{}".format(comp)) == "concurrent",
"If multi_driver is TRUE, NINST_LAYOUT_{} must be concurrent".format(comp))
Expand Down
90 changes: 56 additions & 34 deletions scripts/lib/CIME/case_st_archive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,10 @@ def _get_ninst_info(case, compclass):
for i in range(1,ninst+1):
if ninst > 1:
ninst_strings.append('_' + '{:04d}'.format(i))
else:
ninst_strings.append('')
# KDR This doesn't do what's intended. The result is ninst_strings is not empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

Copy link
Contributor

Choose a reason for hiding this comment

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

This commented code still needs to be removed.

# It has "''" in it.
## else:
## ninst_strings.append('')

logger.debug("ninst and ninst_strings are: {} and {} for {}".format(ninst, ninst_strings, compclass))
return ninst, ninst_strings
Expand All @@ -77,7 +79,7 @@ def _get_ninst_info(case, compclass):
def _get_component_archive_entries(case, archive):
###############################################################################
"""
Each time this is generator function is called, it yields a tuple
Each time this generator function is called, it yields a tuple
(archive_entry, compname, compclass) for one component in this
case's compset components.
"""
Expand All @@ -90,6 +92,9 @@ def _get_component_archive_entries(case, archive):
if archive_entry is not None:
yield(archive_entry, compname, archive_entry.get("compclass"))

if compname not in compset_comps:
logger.debug('Skipping compname %s; it is not in compset_comps' %(compname))
Copy link
Member

Choose a reason for hiding this comment

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

My earlier comment still stands here:

I don't understand the intent of this. Will this ever get triggered? I'm confused because, in the above loop, compname takes on the various values of compset_comps. I can't understand any scenario where compname down here would not be something in compset_comps. Maybe this was a debugging statement that you meant to remove?

Can you either remove these two lines or explain the intent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did put it in as a debugging statement because there were problems
implementing short-term archiving of component esp before it was 'esp'.
There are other debugging statements in this module like this one,
so I thought it would be useful to leave it in. If it doesn't fit with the philosophy
of the scripts, I'm happy to take it out.
Kevin

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind debugging statements in general - I'm just having a hard time seeing how this one could possibly get triggered: compname just loops through compset_comps, so under what circumstances could it ever be the case that compname is not in compset_comps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doh! I moved this from another, superseded routine, where compname was defined
independently of the for loop. I don't know where it would fit now, so I'll delete it.
Thanks for being persistent.


###############################################################################
def _archive_rpointer_files(case, archive, archive_entry, archive_restdir,
datename, datename_is_last):
Expand Down Expand Up @@ -121,27 +126,36 @@ def _archive_rpointer_files(case, archive, archive_entry, archive_restdir,
temp_rpointer_content = rpointer_content
# put in a temporary setting for ninst_strings if they are empty
# in order to have just one loop over ninst_strings below
if rpointer_content is not 'unset':
# KDR 'is not' queries whether they are the same object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment refers to code that is not here, please remove

# We want to compare the strings/contents of the objects
# if rpointer_content is not 'unset':
if rpointer_content != 'unset':
# KDR debugging why rpointer.unset are being created.
logger.info("rpointer_content not unset? {}".format(rpointer_content))
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean for this to stay in the final version? If so, should it be done with something like logger.debug rather than logger.info?

if not ninst_strings:
ninst_strings = ["empty"]
for ninst_string in ninst_strings:
rpointer_file = temp_rpointer_file
rpointer_content = temp_rpointer_content
if ninst_string == 'empty':
ninst_string = ""
for key, value in [('$CASE', casename),
('$DATENAME', datename),
('$NINST_STRING', ninst_string)]:
rpointer_file = rpointer_file.replace(key, value)
rpointer_content = rpointer_content.replace(key, value)

# write out the respect files with the correct contents
rpointer_file = os.path.join(archive_restdir, rpointer_file)
logger.info("writing rpointer_file {}".format(rpointer_file))
f = open(rpointer_file, 'w')
for output in rpointer_content.split(','):
f.write("{} \n".format(output))
f.close()
# KDR indented 4 spaces, up to 'else:' to put under control of 'if ... unset'
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put KDR in comments, this comment should be removed.

for ninst_string in ninst_strings:
rpointer_file = temp_rpointer_file
rpointer_content = temp_rpointer_content
if ninst_string == 'empty':
ninst_string = ""
for key, value in [('$CASE', casename),
('$DATENAME', datename),
('$NINST_STRING', ninst_string)]:
rpointer_file = rpointer_file.replace(key, value)
rpointer_content = rpointer_content.replace(key, value)

# write out the respect files with the correct contents
rpointer_file = os.path.join(archive_restdir, rpointer_file)
logger.info("writing rpointer_file {}".format(rpointer_file))
f = open(rpointer_file, 'w')
for output in rpointer_content.split(','):
f.write("{} \n".format(output))
f.close()
else:
# KDR debugging why rpointer.unset are being created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this logger.info be removed then? ie is it only for debug?

logger.info("rpointer_content unset {}".format(rpointer_content))


###############################################################################
Expand Down Expand Up @@ -192,15 +206,18 @@ def _archive_history_files(case, archive, archive_entry,
rundir = case.get_value("RUNDIR")
for suffix in archive.get_hist_file_extensions(archive_entry):
for i in range(ninst):
if compname == 'dart':
newsuffix = casename + suffix
elif compname.find('mpas') == 0:
newsuffix = compname + '.*' + suffix
if ninst_string:
if compname.find('mpas') == 0:
# Not correct, but MPAS' multi-instance name format is unknown.
Copy link
Member

Choose a reason for hiding this comment

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

fyi I have opened #1931 for this

newsuffix = compname + '.*' + suffix
else:
newsuffix = casename + '.' + compname + ".*" + ninst_string[i] + suffix
else:
if ninst_string:
newsuffix = casename + '.' + compname + ".*" + ninst_string[i] + suffix
else:
newsuffix = casename + '.' + compname + ".*" + suffix
if compname.find('mpas') == 0:
newsuffix = compname + '.*' + suffix
else
newsuffix = casename + '.' + compname + ".*" + suffix

logger.debug("short term archiving suffix is {} ".format(newsuffix))
pfile = re.compile(newsuffix)
histfiles = [f for f in os.listdir(rundir) if pfile.search(f)]
Expand Down Expand Up @@ -327,6 +344,11 @@ def _archive_restarts_date_comp(case, archive, archive_entry,
pattern = compname + suffix + '_'.join(datename.rsplit('-', 1))
pfile = re.compile(pattern)
restfiles = [f for f in os.listdir(rundir) if pfile.search(f)]
# KDR CAM+DART doesn't need special treatment as of 2017-9-1, so this is commented out.
Copy link
Contributor

Choose a reason for hiding this comment

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

just remove this

# if compclass == 'esp' and 'cam' in pattern:
# pattern = suffix + datename
# pfile = re.compile(pattern)
# restfiles = [f for f in os.listdir(rundir) if pfile.search(f)]
else:
pattern = r"{}\.{}\d*.*".format(casename, compname)
if "dart" not in pattern:
Expand Down Expand Up @@ -364,21 +386,21 @@ def _archive_restarts_date_comp(case, archive, archive_entry,
srcfile = os.path.join(rundir, restfile)
destfile = os.path.join(archive_restdir, restfile)
last_restart_file_fn(srcfile, destfile)
logger.info("{} \n{} to \n{}".format(
last_restart_file_fn_msg, srcfile, destfile))
logger.info("{} {} \n{} to \n{}".format(
"datename_is_last", last_restart_file_fn_msg, srcfile, destfile))
for histfile in histfiles_for_restart:
srcfile = os.path.join(rundir, histfile)
destfile = os.path.join(archive_restdir, histfile)
expect(os.path.isfile(srcfile),
"restart file {} does not exist ".format(srcfile))
shutil.copy(srcfile, destfile)
logger.info("copying \n{} to \n{}".format(srcfile, destfile))
logger.info("datename_is_last + histfiles_for_restart copying \n{} to \n{}".format(srcfile, destfile))
else:
# Only archive intermediate restarts if requested - otherwise remove them
if case.get_value('DOUT_S_SAVE_INTERIM_RESTART_FILES'):
srcfile = os.path.join(rundir, restfile)
destfile = os.path.join(archive_restdir, restfile)
logger.info("moving \n{} to \n{}".format(srcfile, destfile))
# KDR redundant? logger.info("moving \n{} to \n{}".format(srcfile, destfile))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment, not needed.

expect(os.path.isfile(srcfile),
"restart file {} does not exist ".format(srcfile))
archive_file_fn(srcfile, destfile)
Expand Down