Derive from pre-made time series, and check directory permissions (#431)

[brianpm-ucar]

Addresses the two tractable parts of #431. Neither needs a change to how the ADF thinks about file locations.

1. Pre-made time series never gained their derived variables

Derivation was welded to the time series creation step: check_derive decides derivability by looking for constituents in a history file (lib/adf_derive.py), and create_time_series skips that whole step when cam_ts_done is true. So a pre-made set of time series could never gain a derived variable — and this happens in your own directory too, which is why it turned out not to be about whose files they are.

Before, with FSNT and FLNT sitting in the directory and RESTOM in diag_var_list:

	 - climatology for RESTOM
	    WARNING: Time series files for variable 'RESTOM' not found.  Script will continue to next variable.
...
ADF diagnostics has completed successfully.        <- rc=0

A silently missing variable and a zero exit code. AdfDiag.derive_from_premade_ts now derives from the time series that are present, so the constituents alone are enough. Both back ends call it — the built-in one and GenTS.

2. Nothing checked permissions

Nothing in lib/ or scripts/ called os.access. A directory this user cannot read reports is_dir() == True and then globs to nothing, so the ADF told the user their history files were missing when they were merely unreadable — sending them to look for the wrong problem:

	Unreadable cam_hist_loc
Before	No history files found for [['cam.h0']]!
After	'...' exists but this user does not have permission to read it!

describe_dir_problem lives in lib/adf_file_utils.py — the stdlib-only module, so it is unit tested in CI — and each caller keeps its own config-key hint. It is used at five read sites (adf_diag.py, adf_gents.py, and three in adf_info.py) and guards the three places the ADF writes:

• a cam_ts_loc that cannot be written now fails immediately with the reason and the remedy, instead of letting every ncrcat fail unexamined (subprocess.run there has no check=, so those failures are invisible) and reporting the files as missing much later;
• a derived variable that would have to be written into someone else's directory is reported as unavailable, rather than raising PermissionError partway through a run:

	 WARNING: ['RESTOM'] would have to be derived, but '<dir>' exists but this user
	     does not have permission to write in it.
	     ** Those variables will be missing. **
	     Set 'cam_ts_done: false' with 'cam_hist_loc' pointing at the history files
	     and 'cam_ts_loc' at a directory you own, to have the ADF make the time
	     series itself.

That last message is the honest statement of what the ADF can and cannot do here, and it points at the configuration that already works today: read someone else's history files, write your own time series. Derived variables have never been a problem in that arrangement, because everything the ADF writes goes to your own space.

No new config keys

derivable_from / derivable_from_cam_chem are read from the existing variable defaults, so no config file changes and every existing config keeps working. _find_constit became find_constit now that it has a caller outside adf_derive.

Testing

Unit tests (5 new, in test_adf_file_utils.py, stdlib-only so CI runs them): a usable directory, a missing path, a file where a directory was wanted, an unreadable directory — asserting the misleading is_dir() == True / empty-glob behavior that motivates the helper — and a read-only directory that is fine to read but reported when need_write=True. Both permission tests skip as root.

End to end on Casper, model-vs-model, two dissimilar cases (a CESM3 alpha 1850 control on cam.h0a, 93 levels; an AMIP run on cam.h0, 32 levels), cam_ts_done: true against pre-made time series:

• RESTOM derived for both cases and both RESTOM climo files written, where before both were silently absent
• derived values exact: max |RESTOM - (FSNT - FLNT)| == 0.0, units and long_name set, constituents dropped from the derived file
• same result through the GenTS back end
• read-only pre-made directory: the warning above, run continues, rc=0
• unreadable cam_hist_loc: the accurate message, immediately
• unwritable cam_ts_loc with cam_ts_done: false: fails immediately with the reason

pytest lib/test/unit_tests 151 passed. pylint --rcfile=lib/test/pylintrc 9.80/10 on both lib/adf_diag.py and lib/adf_info.py (the two CI-linted files touched; gate is 9.5) with no new messages inside the changed hunks. darker --check --revision upstream/main lib/ clean.

Still open on #431

The read-locations vs. write-location split — the catalog idea — is untouched, and neither of these needed it. The remaining limitation is the one the warning names: a derived variable cannot be added to a directory you cannot write in. lib/adf_file_utils.py's find_ts_files is the single chokepoint for time series lookup, so if that turns out to matter in practice, an optional writable location defaulting to the current directory is a small follow-up rather than a rewrite.

🤖 Generated with Claude Code

[brianpm-ucar]

Review round: three defects found and fixed (65f95060)

A fresh Claude-assisted review against the repo's AGENTS.md found three blocking problems in the first version of this PR. All three reproduce, all three are now fixed, and the derivation path has its own test file. Posting the findings because two of them were squarely inside the case this PR claims to fix.

1. Did nothing, silently, with no hist_str configured

The stream loop was for hist_str in as_hist_str_list(hist_strs). With cam_ts_done: true and no hist_str set — legal and ordinary, since there are no history files to name a stream for — adf_info leaves the stream empty (lib/adf_info.py:168, and the comment at :210-215 says the searches downstream are then meant to match any stream). as_hist_str_list("") == [], so the loop body never ran and nothing was printed: the exact "silently missing variable, rc=0" failure this PR is about. find_constit and derive_variable both already accept hist_str=None to mean "any stream", so the fallback is or [None].

Worth noting this is why the original end-to-end testing missed it — both test configs set hist_str explicitly.

2. derivable_from_cam_chem preferred unconditionally

check_derive (lib/adf_derive.py:92-105) uses the CAM-CHEM constituent list only when all of its constituents are present, falling back to derivable_from. The new code was vres.get("derivable_from_cam_chem") or vres.get("derivable_from"), which always takes the CAM-CHEM list. SO4 and SOA are the two variables carrying both. So on an ordinary CAM run with so4_a1/a2/a3 in the pre-made time series, SO4 asked for so4_a5 as well, and derive_variable printed "Not all constituent files present; SO4 cannot be calculated. Please remove SO4 from 'diag_var_list'". _premade_constits now makes the same choice check_derive does, from the time series files.

3. The write check could fail a run that works today

The cam_ts_loc writability check ran before any per-variable work. With cam_ts_done: false, cam_overwrite_ts: false, and a directory that is not writable but already holds the complete set of time series, the existing per-variable branch prints "file was found and overwrite is False. Will use existing file." for every variable and the run completes without writing anything. The check turned that into end_diag_fail. It now runs where a write is actually about to happen — gated on list_of_commands in the built-in back end, and immediately before tsc.execute() in the GenTS one. Verified: that configuration completes rc=0 again, using existing files for all 10 variables.

My apologies for the "every existing config keeps working" line in the original description — that was wrong, and §4.3 is explicit that a change like this has to be called out.

Also fixed

• A read-only pre-made directory that already contains the derived variable no longer gets a "those variables will be missing" warning; what is missing is now worked out before writability is considered.
• describe_dir_problem distinguishes a directory that cannot be reached because its parent is not searchable from one that does not exist — otherwise it reproduced the very confusion it was written to remove.
• The GenTS docstring's Notes and Raises now list derive_from_premade_ts, describe_dir_problem, and the new failure mode (§6.2).

Testing

New lib/test/unit_tests/test_premade_ts_derive.py: 7 checks against a stub AdfDiag with synthetic time series, so no config file or history files are needed. It covers RESTOM derivation, the unset/empty/None stream cases, both constituent-list choices, the already-present case, the unwritable case, and a non-derivable variable being left alone. Reintroducing defects 1 and 2 makes 4 of them fail; with the fixes, 7 pass.

Full suite after the fixes: pytest lib/test/unit_tests 158 passed. pylint --rcfile=lib/test/pylintrc 9.81 on lib/adf_diag.py and 9.80 on lib/adf_info.py (gate 9.5), with no reported line falling inside a changed hunk. darker --check --revision upstream/main lib/ clean. End to end unchanged: RESTOM derived for both cases, both climo files written, and a second run in a row derives nothing and warns about nothing.

Known limitations, stated rather than fixed

• PMID and T are not appended to diag_var_list on this path the way check_derive does for aerosol_zonal_list variables. Derivation itself still works, because derive_variable looks them up against the time series directory, but they do not get climos or plots of their own here, and if they are absent the message names the CAM run rather than the time series directory.
• cam_overwrite_ts is ignored on the pre-made path: an existing derived file is always kept. Defensible for a directory the ADF did not create, but it is an asymmetry with the history path.
• find_constit's loosest fallback pattern is unanchored, so in a shared time series tree the existence check can match another case's file. Pre-existing behaviour, now on a new code path.