Skip to content

Initial Hardening of ConfigOptions#109

Draft
mxkpp wants to merge 3 commits intodevelopmentfrom
maxkipp-config-hardening
Draft

Initial Hardening of ConfigOptions#109
mxkpp wants to merge 3 commits intodevelopmentfrom
maxkipp-config-hardening

Conversation

@mxkpp
Copy link

@mxkpp mxkpp commented Feb 26, 2026

Note: the hardening is currently too strict for the Analysis and Assimilation ("AnA") case.

This case does modify b_date_proc on the fly, and therefore results in the following intentional error currently:

ValueError: Public attr b_date_proc (private attr _b_date_proc) is hardened. It had already been set to non-None value datetime.datetime(2025, 7, 10, 9, 0), and proposed new value is datetime.datetime(2025, 7, 10, 7, 0), which is not equal to the existing value.

The hardening needs to be relaxed for this use case, or, AnA implementation needs to be changed, before this can be merged.

PR Description:

To follow up on the discussion surrounding @lru_cache from #107, this adds hardening to existing class ConfigOptions for the following existing attributes:

b_date_proc
fcst_freq
fcst_input_horizons
aorc_conus_source
aorc_conus_year_url
aorc_alaska_source
aorc_alaska_url
nwm_source
nwm_geogrid
geogrid
geopackage

The hardening is defined in a new setter decorator (can be applied to any setter method). Its rules are that the property can only be set to a non-None value if the current value is None, or if the new value is equal to the current value (via Python == comparison).

The hardening is applied via new setters for those attributes -- they are now properties.

This also adds a type check for b_date_proc, asserting that it is either datetime.datetime or None. Previously, it could also be a str type.

Additions

  • New file general_utils.py with new decorator setter_hardener
  • New setters and getters for attributes described above
  • New constant variable for defining the datetime string format (DRYify)

Removals

Changes

  • 11 attributes moved to properties with standard getter and "hardened" setter, ensuring that values do not change (sameness checked via == comparison) after the first time they are set to a non-None value.
  • b_date_proc may only be a datetime.datetime or None, may not be a str.

Testing

  1. Testing via RTE's run_suite.sh calls is currently in progress as of 2/25/26 evening.

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Linux

…freq, fcst_input_horizons, aorc_conus_source, aorc_conus_year_url, aorc_alaska_source, aorc_alaska_url, nwm_source, nwm_geogrid, geogrid, geopackage. Assert type for b_date_proc.
@mxkpp
Copy link
Author

mxkpp commented Feb 26, 2026

This cannot be merged yet.

As currently mentioned in the PR description, the hardening is currently too strict for the Analysis and Assimilation ("AnA") case.

This case does modify b_date_proc on the fly, and therefore results in the following intentional error currently:

ValueError: Public attr b_date_proc (private attr _b_date_proc) is hardened. It had already been set to non-None value datetime.datetime(2025, 7, 10, 9, 0), and proposed new value is datetime.datetime(2025, 7, 10, 7, 0), which is not equal to the existing value.

The hardening needs to be relaxed for this use case, or, AnA implementation needs to be changed, before this can be merged.

@mxkpp
Copy link
Author

mxkpp commented Feb 26, 2026

Short tests (2-day simulations) using ./run_suite.sh from RTE have finished for one gage for each of the following historical forcing configurations, see below:

Historical CONUS, AORC forcing: no conflict with hardened attributes (they are not mutated after instantiation)
Historical CONUS, NWM forcing: no conflict
Historical Puerto Rico, NWM forcing: no conflict
Historical Hawaii, NWM forcing: no conflict
Historical Alaska, NWM forcing: no conflict

It is known that the non-historical forcing configuration of Analysis & Assimilation ("Ana") does mutate one of these attributes. However, this non-historical forcing configuration does not affect the @lru_cache usage in historical_forcing.py.

I will run longer tests now for the historical cases, to further confirm that the historical forcing usage does not conflict with the hardened attributes.

@mxkpp mxkpp marked this pull request as draft February 27, 2026 13:28
@mxkpp
Copy link
Author

mxkpp commented Feb 27, 2026

This was converted to draft in case the target branch (currently development) should be changed to point to a different target.

@kyle-larkin
Copy link

This cannot be merged yet.

As currently mentioned in the PR description, the hardening is currently too strict for the Analysis and Assimilation ("AnA") case.

This case does modify b_date_proc on the fly, and therefore results in the following intentional error currently:

ValueError: Public attr b_date_proc (private attr _b_date_proc) is hardened. It had already been set to non-None value datetime.datetime(2025, 7, 10, 9, 0), and proposed new value is datetime.datetime(2025, 7, 10, 7, 0), which is not equal to the existing value.

The hardening needs to be relaxed for this use case, or, AnA implementation needs to be changed, before this can be merged.

I think it is likely that the solution needs to be for a relaxed hardening for this use case.

I think the ngen BMI implementation only ever advances, timesteps can't go backwards. And the AnA is backward looking. So the solution as implemented is to adjust the b_date_proc to the earlier date, and then proceed forward, rather than start at the given b_date_proc and go backwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants