Open taddyb33 opened 2 months ago
FYI: Thanks @hellkite500 for pointing this out, but the additions count, and files added, for this PR is overbloated due to copying the Lower Colorado data into test/api/data
.
I'm looking into other methods for reading this data that doesn't result in unneccessary file duplication. If I can't find any, I'll make a custom compose.yaml
file for the LowerColorado data
See below for the additions count from a few LowerColorado test files
@taddyb33 Thanks for the update on LowerColorado. I think the issue lies with the folder structure. We don’t want to copy forcing, USGS, or USACE files into the new folder, and we prefer not to include them in PRs unless absolutely necessary. This increases the clone size and complicates collaboration for other teams.
Is it possible for your PR to be flexible with our config file (for example /test_AnA_V4_HYFeature.yaml
), so we can specify any folder path for forcing files, USGS, etc., or have it read directly from our config? I noticed you connected rfc_channel_forcings
and rfc_geopackage_data
and copied the gpkg and forcings into those folders, but we’d rather provide the file locations ourselves.
I saw the pop-up window allows only a few variables, like start_time
, to be changed, whereas the config files contain many more. Should we be changing those in the test_AnA_V4_HYFeature.yaml
file, and only adjust start_time
in the pop-up?
I sync'd this branch with the latest T-Route commit, and the duplicated test data was removed, bringing the total additions back to what it should be. Answering the above questions:
Is it possible for your PR to be flexible with our config file
As of right now the API does not have integration for static configuration files. Since this couples with Replace and Route, I don't want to accidentally break something from that side of things.
I noticed you connected rfc_channel_forcings and rfc_geopackage_data and copied the gpkg and forcings into those folders
That data format is related to how I coded replace and route's file directory. rfc_channel_forcings
is where the formatted NWPS data is, rfc_geopackage_data
is the v20.1 gpkg folders
I saw the pop-up window allows only a few variables, like start_time, to be changed,
The way that the API works is we use a base_config.yaml
file and modify it using inputs from the POST request. We would need to add more logic to make the API work better with how a user would call it
Thanks @hellkite500 for the review. I'm finally getting back around to completing these tasks and will post updates upon completion with additional context in the PR. Planning on breaking the commits into:
Also, for context. Adding a 👍 reaction to a PR comment means that I've addressed it in a commit. Trying to make sure I hit all of the comments and cowbells
I'm pretty much finished up with addressing the comments (thanks @hellkite500 for the review). I'll be uploading another commit once my tests for the generic LowerColorado testing pass (where you can point to a test file and run T-Route through the API) and tagging reviewers once that's there.
While there were some great comments above, I don't think all of them will be able to make this PR as it means I have to then add scope to Replace and Route. Those additions include:
This function is very much specific to the R&R t-route usage semantics, and also seems to miss an opporutnity to use the troute.config data models to handle the manipulation/updates, which would alleviate the need for this function to link a yaml dict with a params dict.
It wouldn't be too difficult to refactor this to use forecast hours as an input, would make this more generally useful for a wider range of potential applications
It could be helpful to at least point out how the API could be expanded to provide mechanics to expose other configuration variables to be manipulated by the API, which may help the maintainers unfamiliar with fastapi see what long term maintenance and enahncements may look like
We have work scoped out for more T-Route testing support and enhancements to replace and route. I would like to address these in future commits and version control the final working product rather than keep an ever growing PR.
@AminTorabi-NOAA @kumdonoaa This PR is ready for another pass as was able to add an API endpoint to run the LowerColorado. This is using an updated compose.yaml
file with variables defined by compose.env
.
The endpoint is /api/v1/flow_routing/v4/tests/LowerColorado
and takes in a path to the config file of your choice (defaulted to test/LowerColorado_TX_v4/test_AnA_V4_HYFeature.yaml
)
No changes to the code, or the paths of the files are required
It didn't pass Github Actions.
It didn't pass Github Actions.
From what I can see, it needs an approval from a maintainer to run actions
@kumdonoaa Based on our CIROH Science Meeting conversations, I added a general dockerfile to support the T-Route API. It's located in docker/Dockerfile.troute_api
I also moved all compose.yaml files into docker/
and added different .env files to manage the mounted volumes and ports.
This should make testing, and running the API, easier.
Lastly, docs have been updated in doc/api/api_docs.md
to reflect these changes
edit: changed the reference from docker/api/api_docs.md to
doc/api/api_docs.md
@taddyb It seems this file doesn't exist any more at here: test/api/README.md. Also no folder of docker/api/.
@taddyb It seems this file doesn't exist any more at here: test/api/README.md. Also no folder of docker/api/.
Apologies, I mistyped the path name.
Docs are available at doc/api/api_docs.md
, not docker/api/api_docs.md
I re-added the README.md
file in the test/api/
folder with descriptions on how to test the endpoints.
The FIM-S team at RTX has been making additions to T-Route locally to support the Replace and Route tool (https://github.com/NOAA-OWP/hydrovis/tree/ti/Source/RnR). We want to push these changes back to the base t-route repo as per contractual requirements and for the hydrological community to benefit as a whole.
We developed a FastAPI endpoint for T-Route to be run as a service through
localhost:8004
using parameters and forcings. We also have developed a procedure to push a built T-Route image to a GitHub container registry to be used by other services.Additions
main_v04
function within T-Route/api/v1/flow_routing/v4
) to utilize inputs and shared volume mounts to run T-Route as a Servicetest_compose.yaml
and test datatest/api/data
Dockerfile.troute_api
andcompose.yaml
Removals
-
Changes
main_v04
function within following file:src/troute-nwm/src/nwm_routing/__init__.py
Testing
test/api/README.md
Screenshots
Notes
Todos
-
Checklist
Testing checklist
Target Environment support
Accessibility
Other