-
Notifications
You must be signed in to change notification settings - Fork 35
[1pt] PR: Add multiproc to get usgs rating curves script #1647
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Sep 8, 2025
CarsonPruitt-NOAA
approved these changes
Sep 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Going into the FIM 6.0 release, we planned on getting
usgs_rating_curve
files. Then we found a CatFIM problem that triggered some changes to shared functions thatget_usgs_rating_curves
needed. A quick test after the CatFIM change showed it broke getting usgs rating curves. We deferred it until now. We also wanted to add multi proc as it took over 32 hours to run. Multi-processing has now been added to bring this duration down drastically.A couple of other minor updates were done related to this:
shared_function
run_with_mp
andsetup_mp_file_logger
. In implementation of that system, a few updates and adjustments were added/required.run_with_mp
and it's tasks, we went back to adjust all current scripts using this system and fixed them. Minor adjustments were made to all three scripts:make_dem_difs_for_bridges.py
,pull_osm_roads.py
anddownload_fema_nfhl.py
. All three were stub tested to ensure their small code adjustments were fine.get_usgs_rating_curves.py
picked up a major upgrade on logging including more details of what failed, when and details to details to help show what record id's were being processed when failed.tools_shared_functions.py
.A full new valid set of usgs_rating_curve data files was created, will be copied to all enviros and bash_variables.env updated to reflect the new set.
Renamed files:
data\usgs\rating_curve_get_usgs_curves.py
todata\usgs\get_usgs_rating_curves.py
,Files updated related to file name change (all just note updates):
data\nws\preprocess_ahps_nws.py
,data\usgs\preprocess_ahps_usgs.py
,src\src_adjust_usgs_rating_trace.py
,tools\fimr_to_benchmark.py
,tools\generate_nws_lid.py
andtools\rating_curve_comparison.py
Changes
data
bridges\make_dem_dif_for_bridges.py
: Updated for return values torun_with_mp
shared functions.nflh\download_fema_nfhl.py
: Updated for return values torun_with_mp
shared functions.roads\pull_osm_roads.py
: Updated for return values torun_with_mp
shared functions.usgs\get_usgs_rating_curve.py
: as described above.pyproject.toml
: Updated linting rules doc to reflect the new get_usgs_rating_curve.py file name.src\bash_variables.env
: Updated for new path for the new usgs rating curve filessrc\utils\shared_functions
: In addition to the updates mentioned above forrun_with_mp
, the function namedsetup_mp_file_logger
was updated to make an error file to help identify errors that occur. Logging types of both error and critical now show up in the regular log, but are copied into the new "error" log files to help bring the error(s) to attention. A new function namedsetup_file_logger
was added which is nearly identical to the previous existingsetup_mp_file_logger
but is for non multi-processing usages. It has a few critical differences. There is likely a way to merge them. A couple of new small logging related utility functions were also added.tools\tools_shared_functions.py
: Fixed a bug that assumed a particular https response node was returned.Closes: 1578 and 1596
Testing
A very large amounts of tests were performed during development.
make_dem_dif_for_bridges.py
,make_dem_dif_for_bridges.py
andpull_osm_roads.py
were made to ensure their mp changes were valid. Granted minor test adjustments were made to other parts of code in those files to stub test the new changes.Deployment Plan (For FIM developers use)
It has new usgs_rating_curve files and bash_variables update. Needs to be copied to all enviros.
If you are not a FIM dev team member: Please let us know what you need and we can help with it.
If you are a FIM Dev team member:
Please work with the DevOps team and do not just go ahead and do it without some co-ordination.
Copy where you can, assign where you can not, and it is your responsibility to ensure it is done. Please ensure it is completed before the PR is merged.
Has new or updated python packages, PipFile, Pipefile.lock or Dockerfile changes? DevOps can help or take care of it if you want. Just need to know if it is required.
Require new or adjusted data inputs? Does it have a way to version (folder or file dates)?
Please use caution in removing older version unless it is at least two versions ago. Confirm with DevOps if cleanup might be involved.
If new or updated data sets, has the FIM code, including running fim_pipeline.sh, been updated and tested with the new/adjusted data? You can dev test against subsets if you like.
Notes to DevOps Team or others:
Please add any notes that are helpful for us to make sure it is all done correctly. Do not put actual server names or full true paths, just shortcut paths like 'efs..../inputs/, or 'dev1....inputs', etc.
Issuer Checklist (For developer use)
You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.
[_pt] PR: <description>
dev
branch (the default branch), you have a descriptive Feature Branch name using the format:dev-<description-of-change>
(e.g.dev-revise-levee-masking
)dev
branchpre-commit
hooks were run locally4.x.x.x
Merge Checklist (For Technical Lead use only)