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.

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

Update test routing config to alleviate pydantic errors #654

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented Sep 25, 2023

This PR attempts to resolve the GHA failures we're seeing with the master branch of t-route in the integration tests.

Changes

  • Comments out the config parameters causing pydantic errors:
    • supernetwork_parameters -> geo_file_type
    • supernetwork_parameters -> level_pool -> reservoir_parameter_file
    • compute_parameters -> nexus_input_folder
    • compute_parameters -> next_file_pattern_filter

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
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@program-- program-- added bug Something isn't working CI/CD labels Sep 25, 2023
@program-- program-- self-assigned this Sep 25, 2023
@program--
Copy link
Contributor Author

program-- commented Sep 25, 2023

The current issue is a pydantic validation error + a dict default value issue. With the current routing config (in this PR), we get the AttributeError error:

C++ exception with description "AttributeError: 'NoneType' object has no attribute 'get'

At:
  /home/runner/work/ngen/ngen/.venv/lib/python3.10/site-packages/troute/HYFeaturesNetwork.py(30): read_geopkg
  /home/runner/work/ngen/ngen/.venv/lib/python3.10/site-packages/troute/HYFeaturesNetwork.py(140): read_geo_file
  /home/runner/work/ngen/ngen/.venv/lib/python3.10/site-packages/troute/HYFeaturesNetwork.py(233): __init__
  /home/runner/work/ngen/ngen/.venv/lib/python3.10/site-packages/nwm_routing/__main__.py(80): main_v04
" thrown in the test body.

This is because in read_geopkg, when those values are read, if they don't exist then the values default to None instead of {} -- however, a .get() call is immediately called on these, which doesn't exist for None.

If we add shims to the routing config that contain the defaults, then we get a ValidationError from pydantic:

C++ exception with description "ValidationError: 3 validation errors for Config
network_topology_parameters -> waterbody_parameters -> rfc
  extra fields not permitted (type=value_error.extra)
compute_parameters -> data_assimilation_parameters -> reservoir_da -> reservoir_persistence_usace
  extra fields not permitted (type=value_error.extra)
compute_parameters -> data_assimilation_parameters -> reservoir_da -> reservoir_persistence_usgs
  extra fields not permitted (type=value_error.extra)

@program-- program-- marked this pull request as ready for review September 25, 2023 21:50
@hellkite500 hellkite500 merged commit 7316c25 into NOAA-OWP:master Sep 26, 2023
ashleymedin added a commit to ashleymedin/ngen that referenced this pull request Sep 28, 2023
* commit '72f121a5d12762d59c3870efd6d0598f2286886c':
  Fix a CMakelist.txt error.
  Add unit test for logging and other revisions
  Implement a logging function
  cmake: set NetCDF support to default to OFF
  Update test routing config to alleviate pydantic errors (NOAA-OWP#654)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants