Skip to content

Commit 23ded14

Browse files
jirhikerclaude
andcommitted
Config hygiene: validate parameter, flag conflicting flags, dedup source list
Three low-risk improvements to make config mistakes surface early instead of failing silently or far downstream: - validate() now rejects an unknown `parameter` (guarded so empty-parameter sites-only flows still pass), and emits an advisory warning when more than one spatial filter (bbox/county/wkt) or more than one output mode is set — states the code currently accepts but resolves inconsistently. - Orchestration's DIEConfigResource no longer hardcodes the source list for include-list products; it derives from backend.config.SOURCE_KEYS, so a new source can't be silently dropped (removes the "must stay in sync" note). Adds tests/test_config_validation.py. Full suite (297) + dg check defs clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 08679f3 commit 23ded14

3 files changed

Lines changed: 113 additions & 9 deletions

File tree

backend/config.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,54 @@ def validate(self):
439439
self.warn(f"Invalid end date {self.end_date}")
440440
sys.exit(2)
441441

442+
if not self._validate_parameter():
443+
self.warn(
444+
f"Unknown parameter {self.parameter!r}. "
445+
f"Valid parameters: {sorted(PARAMETER_SOURCE_MAP)}"
446+
)
447+
sys.exit(2)
448+
449+
# Advisory only: these states are accepted (the code picks one) but are
450+
# almost always a mistake, so surface them instead of failing silently.
451+
self._warn_spatial_exclusivity()
452+
self._warn_output_mode_exclusivity()
453+
454+
def _validate_parameter(self):
455+
# An empty parameter is valid: sites-only flows don't need one. A set
456+
# parameter must be one the source map knows, otherwise no source can
457+
# ever be resolved for it.
458+
if self.parameter:
459+
return self.parameter in PARAMETER_SOURCE_MAP
460+
return True
461+
462+
def _warn_spatial_exclusivity(self):
463+
# bbox/county/wkt are resolved with inconsistent precedence across
464+
# bbox_bounding_points (bbox first) and bounding_wkt (wkt first), so
465+
# setting more than one silently does different things in different code
466+
# paths. Exactly one (or none, meaning statewide) is intended.
467+
set_filters = [n for n in ("bbox", "county", "wkt") if getattr(self, n)]
468+
if len(set_filters) > 1:
469+
self.warn(
470+
f"Multiple spatial filters set ({', '.join(set_filters)}); set "
471+
"exactly one — resolution precedence differs between code paths."
472+
)
473+
474+
def _warn_output_mode_exclusivity(self):
475+
modes = [
476+
n
477+
for n in (
478+
"output_summary",
479+
"output_timeseries_unified",
480+
"output_timeseries_separated",
481+
)
482+
if getattr(self, n)
483+
]
484+
if len(modes) > 1:
485+
self.warn(
486+
f"Multiple output modes set ({', '.join(modes)}); only the first "
487+
"is used at dump time. Set exactly one."
488+
)
489+
442490
def _extract_date(self, d):
443491
if d:
444492
for fmt in (

orchestration/resources/die_config.py

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import os
22
from typing import Optional
33
import dagster as dg
4-
from backend.config import Config
4+
from backend.config import Config, SOURCE_KEYS
55

66

77
class DIEConfigResource(dg.ConfigurableResource):
@@ -62,14 +62,10 @@ def get_config(self, product: dict, parameter: Optional[str] = None) -> Config:
6262
payload["wkt"] = None
6363

6464
if sources_spec.get("include"):
65-
# NOTE: must stay in sync with backend.config.SOURCE_KEYS — an
66-
# include-list product silently drops any source missing here.
67-
all_sources = [
68-
"bernco", "bor", "cabq", "ebid", "nmbgmr_amp",
69-
"nmed_dwb", "nmose_isc_seven_rivers", "nmose_pod",
70-
"nmose_roswell", "nwis", "pvacd", "wqp",
71-
]
72-
for s in all_sources:
65+
# Enable only the included sources. Derived from the backend's
66+
# canonical source list so a new source can't be silently dropped
67+
# from an include-list product.
68+
for s in SOURCE_KEYS:
7369
payload[f"use_source_{s}"] = s in sources_spec["include"]
7470
elif sources_spec.get("exclude"):
7571
for s in sources_spec["exclude"]:

tests/test_config_validation.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
"""Tests for Config.validate() parameter check and advisory exclusivity guards."""
2+
import pytest
3+
4+
from backend.config import Config, PARAMETER_SOURCE_MAP, SOURCE_KEYS
5+
6+
7+
def _cfg(**attrs):
8+
c = Config()
9+
for k, v in attrs.items():
10+
setattr(c, k, v)
11+
return c
12+
13+
14+
class TestParameterValidation:
15+
def test_valid_parameter_passes(self):
16+
c = _cfg(parameter="waterlevels")
17+
c.validate() # no exit
18+
19+
def test_empty_parameter_ok(self):
20+
# sites-only flows carry no parameter
21+
c = _cfg(parameter="")
22+
c.validate()
23+
24+
def test_unknown_parameter_exits(self):
25+
c = _cfg(parameter="not_a_real_parameter")
26+
with pytest.raises(SystemExit):
27+
c.validate()
28+
29+
def test_validate_parameter_helper(self):
30+
assert _cfg(parameter="arsenic")._validate_parameter() is True
31+
assert _cfg(parameter="")._validate_parameter() is True
32+
assert _cfg(parameter="bogus")._validate_parameter() is False
33+
34+
35+
class TestExclusivityGuards:
36+
def test_single_spatial_filter_no_warn(self, capsys):
37+
# county only — should not trip the multi-filter advisory
38+
c = _cfg(parameter="waterlevels", county="Bernalillo")
39+
c._warn_spatial_exclusivity() # must not raise
40+
41+
def test_multiple_spatial_filters_advisory(self):
42+
c = _cfg(county="Bernalillo", wkt="POLYGON((0 0,0 1,1 1,1 0,0 0))")
43+
# advisory only — does not raise/exit
44+
c._warn_spatial_exclusivity()
45+
46+
def test_multiple_output_modes_advisory(self):
47+
c = _cfg(output_summary=True, output_timeseries_unified=True)
48+
c._warn_output_mode_exclusivity() # advisory, no raise
49+
50+
def test_validate_passes_with_one_mode(self):
51+
c = _cfg(parameter="waterlevels", output_summary=True)
52+
c.validate()
53+
54+
55+
class TestSourceKeysCanonical:
56+
def test_source_keys_cover_parameter_map(self):
57+
# every agency referenced by the parameter map is a real source key
58+
for entry in PARAMETER_SOURCE_MAP.values():
59+
for agency in entry["agencies"]:
60+
assert agency in SOURCE_KEYS

0 commit comments

Comments
 (0)