From b238a8515b133c9b28721a0aed559601e37f4040 Mon Sep 17 00:00:00 2001 From: Brian Medeiros Date: Fri, 4 Sep 2026 15:40:59 -0600 Subject: [PATCH 1/3] Derive from pre-made time series, and check directory permissions Two gaps that get in the way of running the ADF on files someone else produced (#431), neither of which needs a change to how the ADF thinks about file locations. Derived variables were welded to the time series *creation* step: `check_derive` decides derivability by looking for constituents in a history file, and `create_time_series` skips that whole step when `cam_ts_done` is true. A pre-made set of time series therefore never gained its derived variables -- with FSNT and FLNT sitting in the directory and RESTOM requested, the climatology step reported RESTOM as having no time series files and the run still ended "successfully". That happens in the user's own directory too, so it was never really about whose files they are. `derive_from_premade_ts` derives from the time series that are present instead, so the constituents alone are enough, and both time series back ends now call it. Nothing in lib/ or scripts/ called `os.access`, so a directory this user cannot read was reported as a directory with no history files in it -- `is_dir()` returns True for one and its glob comes back empty, which sends the user looking for missing data that is really just unreadable. `describe_dir_problem` distinguishes the cases and each caller keeps its own config-key hint. It also guards the two places the ADF writes: a `cam_ts_loc` that cannot be written now fails immediately with the reason, rather than letting every ncrcat fail unexamined, and a derived variable that would have to be written into someone else's directory is reported as unavailable instead of raising PermissionError partway through a run. Fixes part of #431; the read-locations vs. write-location split discussed there is not needed for either of these. Co-Authored-By: Claude Opus 5 (1M context) --- lib/adf_derive.py | 19 +-- lib/adf_diag.py | 143 +++++++++++++++++++-- lib/adf_file_utils.py | 45 ++++++- lib/adf_gents.py | 35 ++++- lib/adf_info.py | 35 ++--- lib/adf_utils.py | 4 +- lib/test/unit_tests/test_adf_file_utils.py | 102 ++++++++++++++- 7 files changed, 338 insertions(+), 45 deletions(-) diff --git a/lib/adf_derive.py b/lib/adf_derive.py index 43329a3cb..6f2ccca90 100644 --- a/lib/adf_derive.py +++ b/lib/adf_derive.py @@ -152,8 +152,8 @@ def check_derive(self, res, var, case_name, diag_var_list, constit_dict, hist_fi ######## -def _find_constit(ts_dir, case_name, constit, hist_str=None, *, - syr=None, eyr=None): + +def find_constit(ts_dir, case_name, constit, hist_str=None, *, syr=None, eyr=None): """ Locate a constituent's time series file(s) for one case and stream. @@ -245,8 +245,7 @@ def derive_variable(self, case_name, var, res=None, ts_dir=None, constit_matches = {} for constit in constit_list: # Check if the constituent file(s) are present, if so add them to the dict - matches = _find_constit(ts_dir, case_name, constit, hist_str, - syr=syr, eyr=eyr) + matches = find_constit(ts_dir, case_name, constit, hist_str, syr=syr, eyr=eyr) if not matches: continue if utils.ts_files_overlap(matches): @@ -356,8 +355,9 @@ def derive_variable(self, case_name, var, res=None, ts_dir=None, # the whole list: taking [0] would multiply a full-span variable by # a single chunk, which time-axis alignment turns silently into NaN. # Check if PMID is in file: - ds_pmid = self.data.load_dataset(_find_constit(ts_dir, case_name, "PMID", hist_str, - syr=syr, eyr=eyr)) + ds_pmid = self.data.load_dataset( + find_constit(ts_dir, case_name, "PMID", hist_str, syr=syr, eyr=eyr) + ) if not ds_pmid: errmsg = "Missing necessary files for dry air density (rho) " errmsg += "calculation.\nPlease make sure 'PMID' is in the CAM " @@ -369,8 +369,9 @@ def derive_variable(self, case_name, var, res=None, ts_dir=None, return # Check if T is in file: - ds_t = self.data.load_dataset(_find_constit(ts_dir, case_name, "T", hist_str, - syr=syr, eyr=eyr)) + ds_t = self.data.load_dataset( + find_constit(ts_dir, case_name, "T", hist_str, syr=syr, eyr=eyr) + ) if not ds_t: errmsg = "Missing necessary files for dry air density (rho) " errmsg += "calculation.\nPlease make sure 'T' is in the CAM " @@ -436,4 +437,4 @@ def derive_variable(self, case_name, var, res=None, ts_dir=None, ds_final[tvar] = ds_final[tvar].load() ds_final.to_netcdf(derived_file, unlimited_dims='time', mode='w') # End if (all the necessary constituent files exist) -######## \ No newline at end of file +######## diff --git a/lib/adf_diag.py b/lib/adf_diag.py index 664120c03..4306f19b1 100644 --- a/lib/adf_diag.py +++ b/lib/adf_diag.py @@ -95,10 +95,15 @@ # +++++++++++++++++++++++++++++ # Finally, import needed ADF modules: -from adf_file_utils import select_ts_files, ts_files_need_combining +from adf_file_utils import ( + as_hist_str_list, + describe_dir_problem, + select_ts_files, + ts_files_need_combining, +) from adf_web import AdfWeb from adf_dataset import AdfData -from adf_derive import check_derive, derive_variable +from adf_derive import check_derive, derive_variable, find_constit ################# # Helper functions @@ -378,6 +383,100 @@ def get_ts_case_config(self, baseline=False): ######### + def derive_from_premade_ts( + self, case_name, ts_dir, res, hist_strs, *, syr=None, eyr=None + ): + """ + Derive variables that a set of pre-made time series does not contain. + + A run configured with ``cam_ts_done: true`` skips the time series + step, and derivation used to happen only inside that step, decided by + looking for constituents in a history file. A pre-made set therefore + never gained its derived variables: with ``FSNT`` and ``FLNT`` present + and ``RESTOM`` requested, the climatology step reported ``RESTOM`` as + having no time series files and the run still ended "successfully". + This derives from the time series that are there instead, so the + constituents alone are enough. + + Nothing is written when the directory belongs to someone else, since + the derived file is written alongside its constituents. That case is + reported rather than attempted, because the alternative is a + ``PermissionError`` traceback partway through a run. + + Parameters + ---------- + case_name : str + name of the case being processed + ts_dir : str or Path + directory holding the pre-made time series files + res : dict + variable defaults, used for the ``derivable_from`` lists + hist_strs : list + configured history stream(s) for this case + syr, eyr : int, optional + first and last year being processed + + Returns + ------- + None + Writes a time series file for each variable it can derive. + + Notes + ----- + Uses ``self.diag_var_list`` and :func:`derive_variable`. + """ + # Only derivable variables are of interest, and only the ones that are + # not already sitting in the directory: + wanted = {} + for var in self.diag_var_list: + vres = res.get(var, {}) + constit_list = vres.get("derivable_from_cam_chem") or vres.get( + "derivable_from" + ) + if constit_list: + wanted[var] = constit_list + # End if + # End for + if not wanted: + return + # End if + + # The derived file lands next to its constituents, so a directory this + # user cannot write in cannot gain one: + ts_problem = describe_dir_problem(ts_dir, need_write=True) + if ts_problem: + wmsg = f"\t WARNING: {sorted(wanted)} would have to be derived, but" + wmsg += f" {ts_problem}.\n\t ** Those variables will be missing. **\n" + wmsg += "\t Set 'cam_ts_done: false' with 'cam_hist_loc' pointing at" + wmsg += " the history files and 'cam_ts_loc' at a directory you own, to" + wmsg += " have the ADF make the time series itself." + print(wmsg) + self.debug_log(wmsg) + return + # End if + + for hist_str in as_hist_str_list(hist_strs): + for var, constit_list in wanted.items(): + # Already there, from a previous run or from whoever made them: + if find_constit(ts_dir, case_name, var, hist_str, syr=syr, eyr=eyr): + continue + # End if + derive_variable( + self, + case_name, + var, + res, + ts_dir, + constit_list, + hist_str=hist_str, + syr=syr, + eyr=eyr, + ) + # End for + # End for + + ######### + def create_time_series(self, baseline=False): """ Generate time series versions of the CAM history file data. @@ -471,25 +570,38 @@ def run_pool(commands, label): print(f"\n Generating CAM time series files for '{case_name}'...") print(f"\n Writing time series files to {ts_dir}") + # Extract start and end year values: + start_year = start_years[case_idx] + end_year = end_years[case_idx] + # Check if particular case should be processed: if cam_ts_done[case_idx]: emsg = "\tNOTE: Configuration file indicates time series files have been " emsg += f"pre-computed for case '{case_name}'. Will rely on those files directly." print(emsg) + # Derived variables are normally made as part of writing the + # time series, which is exactly the step being skipped here, so + # without this nothing would ever create them and the variable + # would go quietly missing (issue #431): + self.derive_from_premade_ts( + case_name, + ts_dir, + res, + hist_str_list[case_idx], + syr=start_year, + eyr=end_year, + ) continue # End if - # Extract start and end year values: - start_year = start_years[case_idx] - end_year = end_years[case_idx] - # Create path object for the CAM history file(s) location: starting_location = Path(cam_hist_locs[case_idx]) - # Check that path actually exists: - if not starting_location.is_dir(): + # Check that the path exists and can be read: + hist_problem = describe_dir_problem(starting_location) + if hist_problem: emsg = f"Provided {case_type_string} 'cam_hist_loc' directory" - emsg += f" '{starting_location}' not found. Script is ending here." + emsg += f" {hist_problem}. Script is ending here." self.end_diag_fail(emsg) # End if @@ -593,6 +705,19 @@ def run_pool(commands, label): # Use pathlib to create parent directories, if necessary. Path(ts_dir).mkdir(parents=True, exist_ok=True) + # An existing directory this user cannot write in is worth + # stopping for. "ncrcat" would fail once per variable and + # those failures are not inspected, so the run would otherwise + # carry on and only report the files as missing much later: + ts_problem = describe_dir_problem(ts_dir, need_write=True) + if ts_problem: + emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" + emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory you" + emsg += " own, or set 'cam_ts_done: true' to read the time series" + emsg += " that are already there." + self.end_diag_fail(emsg) + # End if + # INPUT NAME TEMPLATE: $CASE.$scomp.[$type.][$string.]$date[$ending] first_file_split = str(hist_files[0]).split(".") if first_file_split[-1] == "nc": diff --git a/lib/adf_file_utils.py b/lib/adf_file_utils.py index f30875e36..03ee63db0 100644 --- a/lib/adf_file_utils.py +++ b/lib/adf_file_utils.py @@ -4,10 +4,12 @@ Kept apart from adf_utils so they can be unit tested without the scientific stack: the ADF unit test workflow installs only PyYAML and pytest, so anything that imports xarray/geocat at module level cannot be exercised in CI. This -module imports nothing but pathlib. +module imports nothing but os and pathlib. Functions --------- +describe_dir_problem(path, need_write=False) + Report why a configured directory cannot be used, permissions included. find_ts_files(ts_loc, pattern, recursive=True) Locate time series files matching a glob pattern under a directory. select_ts_files(fils, syr, eyr) @@ -29,9 +31,50 @@ every existing caller. """ +import os from pathlib import Path +def describe_dir_problem(path, need_write=False): + """ + Report why a configured directory cannot be used, or ``None`` if it can. + + Existence is not the only way a configured path fails. A directory the + user cannot read reports ``is_dir() == True`` and then globs to nothing, + so a caller that only checks existence tells the user their data is + missing when the truth is that it cannot be read -- which is the wrong + thing to go looking for. Reading another user's output makes that the + likely case rather than a rare one. + + Parameters + ---------- + path : str or Path + directory named in the config file + need_write : bool, optional + Whether the ADF has to write into it as well as read it. Default is + ``False``. + + Returns + ------- + str or None + A phrase naming the problem, suitable for appending to a caller's own + message, or ``None`` when the directory is usable. + """ + ppath = Path(path) + if not ppath.is_dir(): + return f"'{ppath}' does not exist, or is not a directory" + # End if + # Listing a directory needs both read and search permission, and a missing + # search bit is the one that produces an empty glob rather than an error: + if not os.access(ppath, os.R_OK | os.X_OK): + return f"'{ppath}' exists but this user does not have permission to read it" + # End if + if need_write and not os.access(ppath, os.W_OK): + return f"'{ppath}' exists but this user does not have permission to write in it" + # End if + return None + + def as_hist_str_list(value): """ Normalize a configured history stream setting to a list. diff --git a/lib/adf_gents.py b/lib/adf_gents.py index 13ce6c968..7d427003b 100644 --- a/lib/adf_gents.py +++ b/lib/adf_gents.py @@ -37,6 +37,7 @@ #ADF modules: from adf_base import AdfError +from adf_file_utils import describe_dir_problem from adf_derive import check_derive, derive_variable #++++++++++++++++++++++++++++++ @@ -196,30 +197,52 @@ def create_time_series_gents(adf, baseline=False): print(f"\n Generating CAM time series files for '{case_name}'...") print(f"\n Writing time series files to {ts_dir}") + start_year = cfg["start_years"][case_idx] + end_year = cfg["end_years"][case_idx] + #Check if particular case should be processed: if cfg["cam_ts_done"][case_idx]: emsg = "\tNOTE: Configuration file indicates time series files have been " emsg += f"pre-computed for case '{case_name}'. Will rely on those files directly." print(emsg) + # Pre-made time series still need their derived variables; see + # AdfDiag.derive_from_premade_ts (issue #431): + adf.derive_from_premade_ts( + case_name, + ts_dir, + adf.variable_defaults, + cfg["hist_str_list"][case_idx], + syr=start_year, + eyr=end_year, + ) continue #End if - start_year = cfg["start_years"][case_idx] - end_year = cfg["end_years"][case_idx] - #Create path object for the CAM history file(s) location: starting_location = Path(cfg["cam_hist_locs"][case_idx]) - #Check that path actually exists: - if not starting_location.is_dir(): + # Check that the path exists and can be read: + hist_problem = describe_dir_problem(starting_location) + if hist_problem: emsg = f"Provided {case_type_string} 'cam_hist_loc' directory" - emsg += f" '{starting_location}' not found. Script is ending here." + emsg += f" {hist_problem}. Script is ending here." adf.end_diag_fail(emsg) #End if #Check if time series directory exists, and if not, then create it: Path(ts_dir).mkdir(parents=True, exist_ok=True) + # An existing directory this user cannot write in is worth stopping for, + # rather than letting GenTS fail once per variable: + ts_problem = describe_dir_problem(ts_dir, need_write=True) + if ts_problem: + emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" + emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory you own," + emsg += " or set 'cam_ts_done: true' to read the time series that are" + emsg += " already there." + adf.end_diag_fail(emsg) + # End if + for hist_str in cfg["hist_str_list"][case_idx]: print(f"\t Processing time series for {case_type_string} {case_name}, " diff --git a/lib/adf_info.py b/lib/adf_info.py index 48984a9ed..c4e784a93 100644 --- a/lib/adf_info.py +++ b/lib/adf_info.py @@ -275,14 +275,15 @@ def __init__(self, config_file, debug=False): starting_location = Path(baseline_hist_locs) print(f"\tChecking history files in '{starting_location}'") - #Check if the history file location exists - if not starting_location.is_dir(): + # Check that the history file location exists and can be read + hist_problem = utils.describe_dir_problem(starting_location) + if hist_problem: msg = "Checking history file location:\n" - msg += f"\tThere is no history file location: '{starting_location}'." + msg += f"\tCannot use the history file location: {hist_problem}." self.debug_log(msg) - emsg = f"{data_name} starting_location: History file location not found!\n" - emsg += "\tTry checking the path 'cam_hist_loc' in 'diag_cam_baseline_climo' " - emsg += "section in your config file is correct..." + emsg = f"{data_name} starting_location: {hist_problem}!\n" + emsg += "\tCheck the path 'cam_hist_loc' in the " + emsg += "'diag_cam_baseline_climo' section of your config file." self.end_diag_fail(emsg) file_list = sorted(starting_location.glob("*" + base_hist_str + ".*.nc")) @@ -491,14 +492,15 @@ def __init__(self, config_file, debug=False): file_list = sorted(starting_location.glob('*'+hist_str_use+'.*.nc')) - #Check if the history file location exists - if not starting_location.is_dir(): + # Check that the history file location exists and can be read + hist_problem = utils.describe_dir_problem(starting_location) + if hist_problem: msg = "Checking history file location:\n" - msg += f"\tThere is no history file location: '{starting_location}'." + msg += f"\tCannot use the history file location: {hist_problem}." self.debug_log(msg) - emsg = f"{case_name} starting_location: History file location not found!\n" - emsg += "\tTry checking the path 'cam_hist_loc' in 'diag_cam_climo' " - emsg += "section in your config file is correct..." + emsg = f"{case_name} starting_location: {hist_problem}!\n" + emsg += "\tCheck the path 'cam_hist_loc' in the 'diag_cam_climo' " + emsg += "section of your config file." self.end_diag_fail(emsg) #Check if there are any history files @@ -917,9 +919,12 @@ def get_climo_yrs_from_ts(self, input_ts_loc, case_name, hist_str=None): #Create "Path" objects: input_location = Path(input_ts_loc) - #Check that time series input directory actually exists: - if not input_location.is_dir(): - errmsg = f"\t ERROR: Time series directory '{input_ts_loc}' not found. Script is exiting." + # Check that the time series input directory exists and can be read: + ts_problem = utils.describe_dir_problem(input_location) + if ts_problem: + errmsg = ( + f"\t ERROR: Time series directory {ts_problem}. Script is exiting." + ) raise AdfError(errmsg) #Normalize the configured history stream(s) into a list: diff --git a/lib/adf_utils.py b/lib/adf_utils.py index 2610ec3ec..858124578 100644 --- a/lib/adf_utils.py +++ b/lib/adf_utils.py @@ -3,7 +3,8 @@ Functions --------- -find_ts_files(), select_ts_files(), ts_files_overlap(), ts_file_span(), +describe_dir_problem(), find_ts_files(), select_ts_files(), ts_files_overlap(), +ts_file_span(), as_hist_str_list(), pick_hist_str() re-exported from adf_file_utils; time series file discovery plain_text_units() @@ -64,6 +65,7 @@ # pylint: disable=unused-import from adf_file_utils import ( as_hist_str_list, + describe_dir_problem, find_ts_files, pick_hist_str, select_ts_files, diff --git a/lib/test/unit_tests/test_adf_file_utils.py b/lib/test/unit_tests/test_adf_file_utils.py index 99cb1f469..54e965ba3 100644 --- a/lib/test/unit_tests/test_adf_file_utils.py +++ b/lib/test/unit_tests/test_adf_file_utils.py @@ -21,10 +21,16 @@ #Add ADF "lib" directory to python path: sys.path.append(_ADF_LIB_DIR) -#adf_file_utils imports nothing but pathlib, so these run in CI, where only -#PyYAML and pytest are installed: -from adf_file_utils import (find_ts_files, select_ts_files, ts_files_overlap, - ts_files_need_combining, ts_file_span) +# adf_file_utils imports nothing but pathlib, so these run in CI, where only +# PyYAML and pytest are installed: +from adf_file_utils import ( + describe_dir_problem, + find_ts_files, + select_ts_files, + ts_files_overlap, + ts_files_need_combining, + ts_file_span, +) #++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ #Main adf_file_utils testing routine, used when script is run directly @@ -507,6 +513,94 @@ def test_select_annual_dates(self): self.assertEqual(select_ts_files([short, long], 1, 40), [long]) + # +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + # describe_dir_problem: reading another user's output makes an + # unreadable directory a likely failure rather than a rare one + # +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + + def test_dir_problem_usable(self): + """ + Check that a readable directory reports no problem. + """ + + with tempfile.TemporaryDirectory() as tmpdir: + self.assertIsNone(describe_dir_problem(tmpdir)) + self.assertIsNone(describe_dir_problem(tmpdir, need_write=True)) + + def test_dir_problem_missing(self): + """ + Check that a path that is not there is reported as such. + """ + + with tempfile.TemporaryDirectory() as tmpdir: + missing = os.path.join(tmpdir, "not_there") + + problem = describe_dir_problem(missing) + + self.assertIsNotNone(problem) + self.assertIn("does not exist", problem) + + def test_dir_problem_is_a_file(self): + """ + Check that a file given where a directory is wanted is reported. + """ + + with tempfile.TemporaryDirectory() as tmpdir: + fname = os.path.join(tmpdir, "case.cam.h0a.T.000101-001112.nc") + open(fname, "w").close() + + self.assertIsNotNone(describe_dir_problem(fname)) + + @unittest.skipIf(os.geteuid() == 0, "root ignores directory permissions") + def test_dir_problem_unreadable(self): + """ + Check that an unreadable directory is reported as unreadable. + + This is the case that motivated the helper: such a directory reports + ``is_dir() == True`` and then globs to nothing, so a caller that only + checks existence tells the user their history files are missing. + """ + + with tempfile.TemporaryDirectory() as tmpdir: + unreadable = Path(tmpdir) / "unreadable" + unreadable.mkdir() + (unreadable / "case.cam.h0a.T.000101-001112.nc").touch() + unreadable.chmod(0o000) + try: + # The misleading behavior this exists to catch: + self.assertTrue(unreadable.is_dir()) + self.assertEqual(find_ts_files(unreadable, "*.nc"), []) + + problem = describe_dir_problem(unreadable) + + self.assertIsNotNone(problem) + self.assertIn("permission to read", problem) + finally: + # Restore, or the temporary directory cannot be cleaned up: + unreadable.chmod(0o755) + + @unittest.skipIf(os.geteuid() == 0, "root ignores directory permissions") + def test_dir_problem_unwritable(self): + """ + Check that a readable but unwritable directory is reported only when + the caller says it needs to write, which is what lets ADF read someone + else's time series while refusing to write into them. + """ + + with tempfile.TemporaryDirectory() as tmpdir: + readonly = Path(tmpdir) / "readonly" + readonly.mkdir() + readonly.chmod(0o555) + try: + self.assertIsNone(describe_dir_problem(readonly)) + + problem = describe_dir_problem(readonly, need_write=True) + + self.assertIsNotNone(problem) + self.assertIn("permission to write", problem) + finally: + readonly.chmod(0o755) + #++++++++++++++++++ #Run unit tests if this script is called directly: From 65f95060f7b5d7817f84f43d40834fb799071ef5 Mon Sep 17 00:00:00 2001 From: Brian Medeiros Date: Fri, 4 Sep 2026 16:09:39 -0600 Subject: [PATCH 2/3] Fix three defects found reviewing the pre-made time series derivation Deriving from pre-made time series did nothing, silently, when no history stream was configured. That is a legal and ordinary setting for 'cam_ts_done: true' -- there are no history files to name a stream for, and adf_info leaves 'base_hist_str' empty in exactly that case, saying in a comment that the searches downstream are then meant to match any stream. Iterating an empty string yields nothing, so the loop body never ran. find_constit and derive_variable both take 'hist_str=None' to mean "any stream", so fall back to that. A variable can declare both 'derivable_from_cam_chem' and 'derivable_from'; SO4 and SOA do. check_derive takes the CAM-CHEM list only when every one of its constituents is present, and falls back to the plain CAM list otherwise. Preferring the CAM-CHEM list outright asked an ordinary CAM run for constituents it never wrote, so SO4 and SOA could not be derived at all. _premade_constits makes the same choice from the time series files. The 'cam_ts_loc' write check ran before any per-variable work, so it failed a run that writes nothing because every file is already there and 'cam_overwrite_ts' is false -- a read-only directory of finished time series, which works today. Check where a write is actually about to happen instead, in both back ends. Also: report which variables are missing only when something really has to be written, so a read-only directory that already holds the derived variable is not complained about; distinguish an unreachable directory from a missing one when the parent is not searchable; and record the new dependencies in the GenTS docstring. test_premade_ts_derive.py covers the derivation path against a stub, so no config file or history files are needed. Four of its checks fail if the first two defects above are reintroduced. Co-Authored-By: Claude Opus 5 (1M context) --- lib/adf_diag.py | 157 +++++++--- lib/adf_file_utils.py | 10 + lib/adf_gents.py | 37 +-- lib/test/unit_tests/test_premade_ts_derive.py | 267 ++++++++++++++++++ 4 files changed, 413 insertions(+), 58 deletions(-) create mode 100644 lib/test/unit_tests/test_premade_ts_derive.py diff --git a/lib/adf_diag.py b/lib/adf_diag.py index 4306f19b1..7e3cea093 100644 --- a/lib/adf_diag.py +++ b/lib/adf_diag.py @@ -425,19 +425,46 @@ def derive_from_premade_ts( ----- Uses ``self.diag_var_list`` and :func:`derive_variable`. """ - # Only derivable variables are of interest, and only the ones that are - # not already sitting in the directory: - wanted = {} + # Which variables could be derived at all. A variable can declare + # both a CAM-CHEM constituent list and a plain CAM one: + derivable = [] for var in self.diag_var_list: vres = res.get(var, {}) - constit_list = vres.get("derivable_from_cam_chem") or vres.get( - "derivable_from" - ) - if constit_list: - wanted[var] = constit_list + if vres.get("derivable_from") or vres.get("derivable_from_cam_chem"): + derivable.append(var) # End if # End for - if not wanted: + if not derivable: + return + # End if + + # An unset stream is legal here: with pre-made time series there are no + # history files to name one, and the searches are then meant to match + # any stream, which find_constit does when given None. + streams = as_hist_str_list(hist_strs) or [None] + + # Work out what is actually missing before writing anything, so that a + # directory the ADF cannot write in is only reported when there is + # something it would have had to write: + todo = [] + for hist_str in streams: + for var in derivable: + # Already there, from a previous run or from whoever made them: + if find_constit(ts_dir, case_name, var, hist_str, syr=syr, eyr=eyr): + continue + # End if + todo.append( + ( + hist_str, + var, + self._premade_constits( + res[var], ts_dir, case_name, hist_str, syr=syr, eyr=eyr + ), + ) + ) + # End for + # End for + if not todo: return # End if @@ -445,8 +472,9 @@ def derive_from_premade_ts( # user cannot write in cannot gain one: ts_problem = describe_dir_problem(ts_dir, need_write=True) if ts_problem: - wmsg = f"\t WARNING: {sorted(wanted)} would have to be derived, but" - wmsg += f" {ts_problem}.\n\t ** Those variables will be missing. **\n" + wmsg = f"\t WARNING: {sorted({var for _, var, _ in todo})} would have" + wmsg += f" to be derived, but {ts_problem}." + wmsg += "\n\t ** Those variables will be missing. **\n" wmsg += "\t Set 'cam_ts_done: false' with 'cam_hist_loc' pointing at" wmsg += " the history files and 'cam_ts_loc' at a directory you own, to" wmsg += " have the ADF make the time series itself." @@ -455,28 +483,68 @@ def derive_from_premade_ts( return # End if - for hist_str in as_hist_str_list(hist_strs): - for var, constit_list in wanted.items(): - # Already there, from a previous run or from whoever made them: - if find_constit(ts_dir, case_name, var, hist_str, syr=syr, eyr=eyr): - continue - # End if - derive_variable( - self, - case_name, - var, - res, - ts_dir, - constit_list, - hist_str=hist_str, - syr=syr, - eyr=eyr, - ) - # End for + for hist_str, var, constit_list in todo: + derive_variable( + self, + case_name, + var, + res, + ts_dir, + constit_list, + hist_str=hist_str, + syr=syr, + eyr=eyr, + ) # End for ######### + def _premade_constits( + self, vres, ts_dir, case_name, hist_str, *, syr=None, eyr=None + ): + """ + Choose which constituent list to derive a variable from. + + A variable can declare both ``derivable_from_cam_chem`` and + ``derivable_from``, and :func:`check_derive` takes the CAM-CHEM list + only when every one of its constituents is present, falling back to the + plain CAM list otherwise. The same choice has to be made here, from + the time series files rather than from a history file: ``SO4`` and + ``SOA`` both carry two lists, so preferring the CAM-CHEM one outright + would ask an ordinary CAM run for constituents it never wrote. + + Parameters + ---------- + vres : dict + variable defaults for the one variable being derived + ts_dir : str or Path + directory holding the pre-made time series files + case_name : str + name of the case being processed + hist_str : str or None + history stream being processed; ``None`` matches any stream + syr, eyr : int, optional + first and last year being processed + + Returns + ------- + list + The constituent names to derive from. The plain CAM list is + returned when neither list is complete, so that + :func:`derive_variable` reports what is missing against the more + likely intent. + """ + cam_chem = vres.get("derivable_from_cam_chem") + if cam_chem and all( + find_constit(ts_dir, case_name, constit, hist_str, syr=syr, eyr=eyr) + for constit in cam_chem + ): + return cam_chem + # End if + return vres.get("derivable_from") or cam_chem or [] + + ######### + def create_time_series(self, baseline=False): """ Generate time series versions of the CAM history file data. @@ -705,19 +773,6 @@ def run_pool(commands, label): # Use pathlib to create parent directories, if necessary. Path(ts_dir).mkdir(parents=True, exist_ok=True) - # An existing directory this user cannot write in is worth - # stopping for. "ncrcat" would fail once per variable and - # those failures are not inspected, so the run would otherwise - # carry on and only report the files as missing much later: - ts_problem = describe_dir_problem(ts_dir, need_write=True) - if ts_problem: - emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" - emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory you" - emsg += " own, or set 'cam_ts_done: true' to read the time series" - emsg += " that are already there." - self.end_diag_fail(emsg) - # End if - # INPUT NAME TEMPLATE: $CASE.$scomp.[$type.][$string.]$date[$ending] first_file_split = str(hist_files[0]).split(".") if first_file_split[-1] == "nc": @@ -872,6 +927,24 @@ def run_pool(commands, label): list_of_hist_commands.append(cmd_remove_history) # End variable loop + # Only now is a write actually required. Checking earlier + # would fail a run that writes nothing because every file is + # already there and 'cam_overwrite_ts' is false -- which is how + # a read-only directory of finished time series works today. + # "ncrcat" would otherwise fail once per variable, and those + # failures are not inspected, so the run would carry on and + # only report the files as missing much later: + if list_of_commands: + ts_problem = describe_dir_problem(ts_dir, need_write=True) + if ts_problem: + emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" + emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory" + emsg += " you own, or set 'cam_ts_done: true' to read the" + emsg += " time series that are already there." + self.end_diag_fail(emsg) + # End if + # End if + # Now run the "ncrcat" subprocesses in parallel: run_pool(list_of_commands, "ncrcat") diff --git a/lib/adf_file_utils.py b/lib/adf_file_utils.py index 03ee63db0..6d2df1ee4 100644 --- a/lib/adf_file_utils.py +++ b/lib/adf_file_utils.py @@ -62,6 +62,16 @@ def describe_dir_problem(path, need_write=False): """ ppath = Path(path) if not ppath.is_dir(): + # An unsearchable parent makes is_dir() report False for a directory + # that is really there, so saying "does not exist" would send the user + # after the wrong problem again: + parent = ppath.parent + if parent != ppath and parent.is_dir() and not os.access(parent, os.X_OK): + return ( + f"'{ppath}' cannot be reached, because this user does not have" + f" permission to search '{parent}'" + ) + # End if return f"'{ppath}' does not exist, or is not a directory" # End if # Listing a directory needs both read and search permission, and a missing diff --git a/lib/adf_gents.py b/lib/adf_gents.py index 7d427003b..8e7b2fc3b 100644 --- a/lib/adf_gents.py +++ b/lib/adf_gents.py @@ -144,16 +144,19 @@ def create_time_series_gents(adf, baseline=False): ------ AdfError If GenTS is not installed, if ``gents_compression`` was given without - ``gents_compression_level``, if a history file directory is missing, - or if ``PS`` is absent from ``diag_var_list`` while model-level - variables are being diagnosed. + ``gents_compression_level``, if a history file directory is missing or + cannot be read, if ``cam_ts_loc`` cannot be written in while there are + files to write, or if ``PS`` is absent from ``diag_var_list`` while + model-level variables are being diagnosed. Notes ----- Uses ``adf.get_basic_info``, ``adf.get_ts_case_config``, ``adf.diag_var_list``, ``adf.variable_defaults``, ``adf.num_procs``, - ``adf.user`` and ``adf.end_diag_fail``, plus ``check_derive`` and - ``derive_variable`` from :mod:`adf_derive`. + ``adf.user``, ``adf.end_diag_fail`` and + ``adf.derive_from_premade_ts``, plus ``check_derive`` and + ``derive_variable`` from :mod:`adf_derive` and ``describe_dir_problem`` + from :mod:`adf_file_utils`. """ HFCollection, TSCollection = _import_gents() @@ -232,17 +235,6 @@ def create_time_series_gents(adf, baseline=False): #Check if time series directory exists, and if not, then create it: Path(ts_dir).mkdir(parents=True, exist_ok=True) - # An existing directory this user cannot write in is worth stopping for, - # rather than letting GenTS fail once per variable: - ts_problem = describe_dir_problem(ts_dir, need_write=True) - if ts_problem: - emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" - emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory you own," - emsg += " or set 'cam_ts_done: true' to read the time series that are" - emsg += " already there." - adf.end_diag_fail(emsg) - # End if - for hist_str in cfg["hist_str_list"][case_idx]: print(f"\t Processing time series for {case_type_string} {case_name}, " @@ -331,6 +323,19 @@ def create_time_series_gents(adf, baseline=False): tsc = tsc.add_attrs({"adf_user": adf.user, "hist_file_locs": str(starting_location)}) + # Only now is a write actually required, and only for the files + # GenTS still has to make. Checking before this point would fail a + # run that writes nothing, which is how a read-only directory of + # finished time series works today: + ts_problem = describe_dir_problem(ts_dir, need_write=True) + if ts_problem: + emsg = f"Provided {case_type_string} 'cam_ts_loc' directory" + emsg += f" {ts_problem}.\n\tSet 'cam_ts_loc' to a directory you own," + emsg += " or set 'cam_ts_done: true' to read the time series that are" + emsg += " already there." + adf.end_diag_fail(emsg) + # End if + print(f"\t - generating {len(tsc)} time series file(s) with GenTS") tsc.create_directories() tsc.execute(show_progress=show_progress) diff --git a/lib/test/unit_tests/test_premade_ts_derive.py b/lib/test/unit_tests/test_premade_ts_derive.py new file mode 100644 index 000000000..5ceaa1a14 --- /dev/null +++ b/lib/test/unit_tests/test_premade_ts_derive.py @@ -0,0 +1,267 @@ +""" +Collection of python unit tests for deriving variables from pre-made time +series, i.e. the 'cam_ts_done: true' path through AdfDiag. + +These exercise AdfDiag.derive_from_premade_ts against a stub object rather +than a configured run, so no config file or history files are needed. They +import adf_diag, which needs the scientific stack, so they do not run in the +ADF unit test workflow (see adf_file_utils for what does). +""" + +# +++++++++++++++++++++++ +# Import required modules +# +++++++++++++++++++++++ + +import unittest +import sys +import os +import os.path +import tempfile +from pathlib import Path + +import numpy as np +import xarray as xr + +# Set relevant path variables: +_CURRDIR = os.path.abspath(os.path.dirname(__file__)) +_ADF_LIB_DIR = os.path.join(_CURRDIR, os.pardir, os.pardir) + +# Add ADF "lib" directory to python path: +sys.path.append(_ADF_LIB_DIR) + +from adf_diag import AdfDiag + +_CASE = "mycase" +_STREAM = "cam.h0a" +_SPAN = "000101-000212" + + +def _write_ts(ts_dir, var, value, stream=_STREAM): + """Write a minimal ADF-named time series file holding one variable.""" + time = xr.date_range( + "0001-01-01", periods=24, freq="MS", calendar="noleap", use_cftime=True + ) + data = np.full((24, 2, 2), value, dtype="float32") + ds = xr.Dataset( + {var: (("time", "lat", "lon"), data)}, + coords={"time": time, "lat": [-45.0, 45.0], "lon": [0.0, 180.0]}, + ) + ds[var].attrs = {"units": "W/m2", "long_name": f"{var} for testing"} + fname = Path(ts_dir) / f"{_CASE}.{stream}.{var}.{_SPAN}.nc" + ds.to_netcdf(fname) + return fname + + +class _StubData: + """Stands in for AdfData, whose only use here is opening the files.""" + + @staticmethod + def load_dataset(fils): + """Open the constituent files the way AdfData does.""" + if not fils: + return None + return xr.open_mfdataset([str(f) for f in fils], decode_times=True) + + +class _StubAdf: + """The parts of AdfDiag that derive_from_premade_ts actually touches.""" + + def __init__(self, diag_var_list): + self.diag_var_list = diag_var_list + self.data = _StubData() + self.debug_msgs = [] + + def debug_log(self, msg): + """Record instead of writing a log file.""" + self.debug_msgs.append(msg) + + # Borrow the real implementations under test: + derive_from_premade_ts = AdfDiag.derive_from_premade_ts + _premade_constits = AdfDiag._premade_constits + + +# ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ +# Main pre-made time series derivation testing routine +# ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + + +class PremadeTsDeriveTestRoutine(unittest.TestCase): + """ + Unit tests for deriving variables from time series the ADF did not make, + which is the case a 'cam_ts_done: true' run is in. + """ + + def test_derives_restom(self): + """ + Check that RESTOM is derived from pre-made FSNT and FLNT. + """ + + res = {"RESTOM": {"derivable_from": ["FSNT", "FLNT"]}} + with tempfile.TemporaryDirectory() as tmpdir: + _write_ts(tmpdir, "FSNT", 240.0) + _write_ts(tmpdir, "FLNT", 235.0) + adf = _StubAdf(["RESTOM"]) + + adf.derive_from_premade_ts(_CASE, tmpdir, res, [_STREAM], syr=1, eyr=2) + + out = Path(tmpdir) / f"{_CASE}.{_STREAM}.RESTOM.{_SPAN}.nc" + self.assertTrue(out.is_file()) + with xr.open_dataset(out) as ds: + self.assertEqual(float(ds["RESTOM"].mean()), 5.0) + + def test_derives_with_no_stream_configured(self): + """ + Check that derivation still happens when no history stream is set. + + With pre-made time series there are no history files to name a stream + for, so 'hist_str' is legitimately empty and the ADF records it as "". + Iterating that yields nothing, so a stream loop alone would skip the + derivation without saying a word. + """ + + res = {"RESTOM": {"derivable_from": ["FSNT", "FLNT"]}} + for hist_strs in ("", [], None): + with self.subTest(hist_strs=hist_strs): + with tempfile.TemporaryDirectory() as tmpdir: + _write_ts(tmpdir, "FSNT", 240.0) + _write_ts(tmpdir, "FLNT", 235.0) + adf = _StubAdf(["RESTOM"]) + + adf.derive_from_premade_ts( + _CASE, tmpdir, res, hist_strs, syr=1, eyr=2 + ) + + out = Path(tmpdir) / f"{_CASE}.{_STREAM}.RESTOM.{_SPAN}.nc" + self.assertTrue( + out.is_file(), f"nothing derived for hist_strs={hist_strs!r}" + ) + + def test_plain_cam_constituents_preferred_when_cam_chem_incomplete(self): + """ + Check that a variable carrying both constituent lists uses the plain + CAM one when the CAM-CHEM constituents are not all present. + + SO4 and SOA declare both. Preferring 'derivable_from_cam_chem' + outright would ask an ordinary CAM run for constituents it never + wrote, so nothing would be derived. + """ + + res = { + "SO4": { + "derivable_from": ["so4_a1", "so4_a2"], + "derivable_from_cam_chem": ["so4_a1", "so4_a2", "so4_a5"], + } + } + with tempfile.TemporaryDirectory() as tmpdir: + _write_ts(tmpdir, "so4_a1", 1.0) + _write_ts(tmpdir, "so4_a2", 2.0) + adf = _StubAdf(["SO4"]) + + chosen = adf._premade_constits( + res["SO4"], tmpdir, _CASE, _STREAM, syr=1, eyr=2 + ) + + self.assertEqual(chosen, ["so4_a1", "so4_a2"]) + + def test_cam_chem_constituents_used_when_all_present(self): + """ + Check that the CAM-CHEM list wins once all of its constituents are + there, which is the choice check_derive makes from a history file. + """ + + res = { + "SO4": { + "derivable_from": ["so4_a1", "so4_a2"], + "derivable_from_cam_chem": ["so4_a1", "so4_a2", "so4_a5"], + } + } + with tempfile.TemporaryDirectory() as tmpdir: + for constit in ("so4_a1", "so4_a2", "so4_a5"): + _write_ts(tmpdir, constit, 1.0) + adf = _StubAdf(["SO4"]) + + chosen = adf._premade_constits( + res["SO4"], tmpdir, _CASE, _STREAM, syr=1, eyr=2 + ) + + self.assertEqual(chosen, ["so4_a1", "so4_a2", "so4_a5"]) + + def test_nothing_said_when_derived_file_already_there(self): + """ + Check that an unwritable directory is not complained about when it + already holds the derived variable. + + Reading someone else's finished time series is the whole point, so a + directory that needs nothing written to it must not be reported. + """ + + res = {"RESTOM": {"derivable_from": ["FSNT", "FLNT"]}} + with tempfile.TemporaryDirectory() as tmpdir: + readonly = Path(tmpdir) / "readonly" + readonly.mkdir() + _write_ts(readonly, "FSNT", 240.0) + _write_ts(readonly, "FLNT", 235.0) + _write_ts(readonly, "RESTOM", 5.0) + readonly.chmod(0o555) + try: + adf = _StubAdf(["RESTOM"]) + + adf.derive_from_premade_ts( + _CASE, readonly, res, [_STREAM], syr=1, eyr=2 + ) + + self.assertEqual(adf.debug_msgs, []) + finally: + readonly.chmod(0o755) + + @unittest.skipIf(os.geteuid() == 0, "root ignores directory permissions") + def test_unwritable_directory_reported_once(self): + """ + Check that a directory that cannot gain the derived file says so, + naming the variable, rather than raising PermissionError. + """ + + res = {"RESTOM": {"derivable_from": ["FSNT", "FLNT"]}} + with tempfile.TemporaryDirectory() as tmpdir: + readonly = Path(tmpdir) / "readonly" + readonly.mkdir() + _write_ts(readonly, "FSNT", 240.0) + _write_ts(readonly, "FLNT", 235.0) + readonly.chmod(0o555) + try: + adf = _StubAdf(["RESTOM"]) + + adf.derive_from_premade_ts( + _CASE, readonly, res, [_STREAM], syr=1, eyr=2 + ) + + self.assertEqual(len(adf.debug_msgs), 1) + self.assertIn("RESTOM", adf.debug_msgs[0]) + self.assertIn("permission to write", adf.debug_msgs[0]) + finally: + readonly.chmod(0o755) + + def test_non_derivable_variables_ignored(self): + """ + Check that a variable with no 'derivable_from' is left alone. + """ + + res = {"TS": {"colormap": "Reds"}} + with tempfile.TemporaryDirectory() as tmpdir: + _write_ts(tmpdir, "TS", 288.0) + before = sorted(p.name for p in Path(tmpdir).glob("*.nc")) + adf = _StubAdf(["TS"]) + + adf.derive_from_premade_ts(_CASE, tmpdir, res, [_STREAM], syr=1, eyr=2) + + self.assertEqual(sorted(p.name for p in Path(tmpdir).glob("*.nc")), before) + + +# ++++++++++++++++++ + +# Run unit tests if this script is called directly: +if __name__ == "__main__": + unittest.main() + +############# +# End of file From c2e9d5c0e9c1d2e2717f015437b68c8a71859df9 Mon Sep 17 00:00:00 2001 From: Brian Medeiros Date: Fri, 4 Sep 2026 16:19:32 -0600 Subject: [PATCH 3/3] Skip the pre-made time series tests when the stack is absent The new tests imported numpy, xarray and adf_diag at module level, so collecting them failed the unit test workflow on all five python versions: it installs only PyYAML and pytest. Guard the imports and skip the class the way test_adf_derive.py already does, and guard the class body too, since it borrows the methods under test off AdfDiag and runs even when the tests themselves are skipped. Checked against a venv holding only PyYAML and pytest, the way CI builds it: 69 passed, 68 skipped, no collection error. Co-Authored-By: Claude Opus 5 (1M context) --- lib/test/unit_tests/test_premade_ts_derive.py | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/lib/test/unit_tests/test_premade_ts_derive.py b/lib/test/unit_tests/test_premade_ts_derive.py index 5ceaa1a14..4303dc130 100644 --- a/lib/test/unit_tests/test_premade_ts_derive.py +++ b/lib/test/unit_tests/test_premade_ts_derive.py @@ -3,9 +3,11 @@ series, i.e. the 'cam_ts_done: true' path through AdfDiag. These exercise AdfDiag.derive_from_premade_ts against a stub object rather -than a configured run, so no config file or history files are needed. They -import adf_diag, which needs the scientific stack, so they do not run in the -ADF unit test workflow (see adf_file_utils for what does). +than a configured run, so no config file or history files are needed. + +NOTE: adf_diag imports xarray and the rest of the scientific stack, so these +are skipped in CI, which installs only PyYAML and pytest. They run in a full +ADF environment. """ # +++++++++++++++++++++++ @@ -19,9 +21,6 @@ import tempfile from pathlib import Path -import numpy as np -import xarray as xr - # Set relevant path variables: _CURRDIR = os.path.abspath(os.path.dirname(__file__)) _ADF_LIB_DIR = os.path.join(_CURRDIR, os.pardir, os.pardir) @@ -29,7 +28,17 @@ # Add ADF "lib" directory to python path: sys.path.append(_ADF_LIB_DIR) -from adf_diag import AdfDiag +# adf_diag imports xarray and the rest of the scientific stack, so these are +# skipped in CI, which installs only PyYAML and pytest. They run in a full ADF +# environment. +try: + import numpy as np + import xarray as xr + from adf_diag import AdfDiag + + _HAS_ADF_DIAG = True +except ImportError: + _HAS_ADF_DIAG = False _CASE = "mycase" _STREAM = "cam.h0a" @@ -75,9 +84,13 @@ def debug_log(self, msg): """Record instead of writing a log file.""" self.debug_msgs.append(msg) - # Borrow the real implementations under test: - derive_from_premade_ts = AdfDiag.derive_from_premade_ts - _premade_constits = AdfDiag._premade_constits + # Borrow the real implementations under test. Guarded, because the class + # body still runs when the scientific stack is missing and the tests + # themselves are skipped: + if _HAS_ADF_DIAG: + derive_from_premade_ts = AdfDiag.derive_from_premade_ts + _premade_constits = AdfDiag._premade_constits + # End if # ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ @@ -85,6 +98,7 @@ def debug_log(self, msg): # ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ +@unittest.skipUnless(_HAS_ADF_DIAG, "adf_diag dependencies not available") class PremadeTsDeriveTestRoutine(unittest.TestCase): """ Unit tests for deriving variables from time series the ADF did not make,