RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837

taddyb33 · 2024-09-03T18:00:26Z

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

exposure of the main_v04 function within T-Route
A FastAPI endpoint (/api/v1/flow_routing/v4) to utilize inputs and shared volume mounts to run T-Route as a Service
an API doc spec and Quickstart file to get the container up and running
A test compose file, test_compose.yaml and test data test/api/data
A docker compose setup for T-Route using the Dockerfile.troute_api and compose.yaml

Removals

Changes

exposure of the main_v04 function within following file: src/troute-nwm/src/nwm_routing/__init__.py

from .__main__ import main_v04

Testing

Follow the steps within test/api/README.md

Screenshots

Notes

For an example docker build, see https://github.com/taddyb33/t-route-dev/pkgs/container/t-route-dev

Todos

Checklist

Testing checklist

Target Environment support

Windows
Linux
Browser

Accessibility

Keyboard friendly
Screen reader friendly

Other

migrated fastapi wrapper to the troute branch

…into migration

Migration

…into migration

Documentation

added test files

AminTorabi-NOAA · 2024-09-06T18:08:38Z

test/api/README.md

+
+To use these files, follow the steps below:
+
+1. Copy the `test_compose.yaml` file in the root project dir 


Can you be more specific with root dir? Do you mean in the src folder or lets say we are running LowerColorado in that folder

Sure, I changed this to the following line:

Copy the `test_compose.yaml` file in the base project dir (/t-route)

Let me know if this should be more verbose.

Edit: originally commented with my personal GitHub account. had to switch to my RTX one

AminTorabi-NOAA · 2024-09-06T18:13:50Z

test/api/README.md

+- num_forecast_days=5
+5. Click execute
+6. A Status 200 code means the run ran, and test/api/data/troute_output will be populated in the `{lid}/` folder
+


I am getting below error

Can you post a screenshot of the the terminal that has docker compose running (where you ran docker compose -f test_compose.yaml up)?

I just ran through the test case and got the following output, which matches what T-Route would show if I were to be running the code locally:

My GET request looks like:

troute-1 | INFO: 172.22.0.1:54084 - "GET /api/v1/flow_routing/v4/?lid=CAGM7&feature_id=2930769&hy_id=1074884&initial_start=0&start_time=2024-08-24T00%3A00%3A00&num_forecast_days=5 HTTP/1.1" 200 OK

The issue was that the test_compose.yaml file was in the wrong directory, but it's working now.

A couple more questions/comments:

When I run it, a browser opens to http://localhost:8000/, but I have to manually change it to http://localhost:8004/docs to display the correct page. Is there a way to open the correct page directly?

Could you provide an example of how to run the Lower Colorado test using //t-route/test/LowerColorado_TX_v4/test_AnA_V4_HYFeature.yaml?

Note: Previously typed in wrong place

When I run it, a browser opens to http://localhost:8000/, but I have to manually change it to http://localhost:8004/docs to display the correct page. Is there a way to open the correct page directly?

I changed the test_compose.yaml file to point to port 8000. The reason it was on port 8004, rather than 8000 to begin with, was because of testing with the Replace and Route application. Since this compose file is local to T-Route, changing the FastAPI port back to 8000 will have no effect on RnR.

As Amin mentioned, testing your application with //t-route/test/LowerColorado_TX_v4/test_AnA_V4_HYFeature.yaml would be more relevant for us to review your PR, as it will help determine whether the T-Route Docker image produces the same output as the current T-Route version and behaves as expected in terms of compute speed and stability.

Thanks, @kumdonoaa and @AminTorabi-NOAA for the suggestion. I just pushed updates to the test/api/README.md file for how to run this API with the LowerColorado data.

Some things to note:

I copied the LowerColorado channel forcings and v20.1 hydrofabric gpkg to the same directories that the test/api/data is located. This was to prevent having to change the mounted volume location within the test_compose.yaml file

You'll notice in the instructions I added (see below) the feature_id and hy_id are the same. I put this this way because of how RnR calls this API. RnR saves it's geopackages based on their feature_id of their downstream point, and thus we pass in the feature ID to t-route so it knows where to look for the gpkg data. In the LowerColorado case, we just want to make sure we know where the data.

- lid=LowerColorado - feature_id=2420801 - hy_id=2420801 - initial_start=0 - start_time=2023-04-01T00:00:00 - num_forecast_days=2

The num_forecast days is set to 2 since the data is from 4/01/23-4/03/23

…or API

taddyb33 · 2024-09-09T16:24:57Z

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

AminTorabi-NOAA · 2024-09-09T16:30:31Z

@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?

The non-editable flag: ./compiler.sh no-e is no longer necessary (see NOAA-OWP#675 for details)

… rebase

Removing lower colorado duplicated files

taddyb33 · 2024-09-09T18:56:55Z

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

hellkite500 · 2024-09-10T16:13:24Z

.gitignore

+data/ 
+output
+test_compose.yaml
+src/app/.ruff_cache


add new line for posix

hellkite500 · 2024-09-10T16:18:35Z

Dockerfile.troute_api

+WORKDIR "/t-route/"
+COPY . "/t-route/"
+
+RUN ln -s /usr/lib64/gfortran/modules/netcdf.mod /usr/include/openmpi-x86_64/netcdf.mod


use the ENV var to point to the include dir
https://github.com/NOAA-OWP/t-route/blob/470c767428f130d79743feed0d792cceafbdcd6d/compiler.sh#L45C15-L45C32

just an FYI, this was taken from docker/Dockerfile.troute https://github.com/NOAA-OWP/t-route/blob/master/docker/Dockerfile.troute#L10C1-L10C88

if I'm making a change to the troute_api container, then the regular container should reflect the same addition

hellkite500 · 2024-09-10T16:22:47Z

Dockerfile.troute_api

+
+RUN uv pip install --no-cache-dir -r requirements.txt
+
+ENV WITH_EDITABLE=true


I don't think an editable build is really needed here

hellkite500 · 2024-09-10T16:22:53Z

Dockerfile.troute_api

+
+RUN uv pip install --no-cache-dir -r requirements-app.txt
+
+WORKDIR "/t-route/src/"


posix newline

hellkite500 · 2024-09-11T13:26:35Z

build.sh

@@ -0,0 +1,18 @@
+#!/bin/bash
+
+if ! docker login --username ${GH_USERNAME} --password ${GH_TOKEN} ghcr.io; then


This needs some additional context, probably in a readme, which describes what is accomplished with this script and what the ghcr.io resource is. Probably also good to describe how to build and use locally without using ghrc.

This script is used to push a built T-Route container to their Github Container Registry. After more thought, I think it's best to delete this script and use CI/CD with github secrets set by maintainers to update docker containers rather than manual builds using ENV credentials.

thoughts?

hellkite500 · 2024-09-12T13:25:01Z

src/app/core/base_config.yaml

+        stream_output_directory: /t-route/output/{}
+        stream_output_time: 1 #[hr]
+        stream_output_type: '.nc' #please select only between netcdf '.nc' or '.csv' or '.pkl'
+        stream_output_internal_frequency: 60 #[min] it should be order of 5 minutes. For instance if you want to output every hour put 60    


More cowbell!

hellkite500 · 2024-09-12T13:27:31Z

src/app/schemas.py

+from pydantic import BaseModel
+
+
+class TRouteOuput(BaseModel):


TRouteOutput is perhaps a misleading name implying this class/return contains actually routed output data. I would rename this to something like TRouteStatus since this is really just status messaging about the run, but doesn't contain model output.

hellkite500 · 2024-09-12T13:27:42Z

src/troute-nwm/src/nwm_routing/__init__.py

@@ -0,0 +1 @@
+from .__main__ import main_v04


More Cowbell!

hellkite500 · 2024-09-12T13:28:56Z

test/api/test_compose.yaml

+      interval: 30s
+      timeout: 5s
+      retries: 3
+      start_period: 5s


More cowbell!

hellkite500 · 2024-09-12T13:32:54Z

test/api/README.md

@@ -0,0 +1,22 @@
+# Testing the T-Route FastAPI Extension


Would like to ultimately see some automated unit tests of the API -- setup the instance with the test_compose inside a test framework's init/setup (e.g. pytest) and and create then send requests to the server and verify the expected return messages at least.

This is scoped work in this current quarter from RTX (10/01-12/17)! We are to enhance all unit tests within T-Route, including this API feature

taddyb · 2024-10-01T20:54:22Z

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:

Generic case with staged data
General comments (cowbell)
Documentation

taddyb · 2024-10-02T17:51:51Z

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

…into development

taddyb · 2024-10-08T18:33:54Z

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.

taddyb · 2024-10-09T14:19:14Z

@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

kumdonoaa · 2024-10-09T14:23:14Z

It didn't pass Github Actions.

taddyb · 2024-10-09T14:30:54Z

It didn't pass Github Actions.

From what I can see, it needs an approval from a maintainer to run actions

https://github.com/NOAA-OWP/t-route/actions/runs/10779259029 (The first PR)
https://github.com/NOAA-OWP/t-route/actions/runs/11256909963 (The latest code commit)

taddyb · 2024-10-21T18:30:56Z

@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

kumdonoaa · 2024-11-04T20:42:49Z

@taddyb It seems this file doesn't exist any more at here: test/api/README.md. Also no folder of docker/api/.

taddyb · 2024-11-05T15:03:02Z

@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.

taddyb added 12 commits August 20, 2024 20:49

migrated fastapi wrapper to the troute branch

c22c82a

Merge pull request #1 from taddyb33/migration

43d4489

migrated fastapi wrapper to the troute branch

ensuring this dir is here

c7cc896

added restart files to remove dry conditions for RFC routing

a2c826f

Merge branch 'development' of https://github.com/taddyb33/t-route-dev …

4eba8a9

…into migration

Merge pull request #2 from taddyb33/migration

60f2c7c

Migration

Added docs for t-route api release

7306c68

Merge branch 'development' of https://github.com/taddyb33/t-route-dev …

60986b8

…into migration

linted code?

510386e

Merge pull request #3 from taddyb33/migration

3dfb149

Documentation

added test files

0a6f481

Merge pull request #4 from taddyb33/test_docs

627d57f

added test files

taddyb33 mentioned this pull request Sep 3, 2024

RTX Code Release (PI-3): Enhancements to Replace and Route to support enterprise hydrofabric and new NWPS api capabilities in near-real time NOAA-OWP/hydrovis#865

Open

21 tasks

AminTorabi-NOAA reviewed Sep 6, 2024

View reviewed changes

taddyb added 3 commits September 6, 2024 14:20

added author name in app/main.py, added specifics to testing README f…

2cf4c3f

…or API

changed port from 8004 to 8000

81fe066

added the lower colorado forcing and domain files into the api test

d0f8575

taddyb and others added 8 commits September 9, 2024 14:40

Merge branch 'NOAA-OWP:master' into development

35e4b6c

if the folder doesn't exist

ffb9f96

handling edge cases

b28544f

Update readme.md: removed obsolete no-e option (NOAA-OWP#843)

2ecd279

The non-editable flag: ./compiler.sh no-e is no longer necessary (see NOAA-OWP#675 for details)

Speeding up writing output (NOAA-OWP#841)

2deff3c

Merge branch 'rebase' of https://github.com/taddyb33/t-route-dev into…

51b2253

… rebase

removing lower colorado duplicated data

b3960e0

Merge pull request #5 from taddyb33/rebase

5e1a004

Removing lower colorado duplicated files

hellkite500 reviewed Sep 12, 2024

View reviewed changes

taddyb33 and others added 3 commits October 7, 2024 15:17

Delete data/troute_restart/.gitkeep

884c101

completed general PR comments and formatting

ea96782

Merge branch 'development' of https://github.com/taddyb33/t-route-dev …

7e5dc45

…into development

added general LowerColorado Test case for API

4fc70fa

reorganized dockerfiles

8ca681a

updated docs, added README

7c20da5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837

RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837

taddyb33 commented Sep 3, 2024 •

edited

Loading

AminTorabi-NOAA Sep 6, 2024

taddyb33 Sep 6, 2024

AminTorabi-NOAA Sep 6, 2024

taddyb33 Sep 6, 2024

AminTorabi-NOAA Sep 6, 2024

taddyb33 Sep 6, 2024

kumdonoaa Sep 6, 2024

taddyb33 Sep 6, 2024

taddyb33 commented Sep 9, 2024

AminTorabi-NOAA commented Sep 9, 2024

taddyb33 commented Sep 9, 2024

hellkite500 Sep 10, 2024

hellkite500 Sep 10, 2024

taddyb Oct 4, 2024

hellkite500 Sep 10, 2024

hellkite500 Sep 10, 2024

hellkite500 Sep 11, 2024

taddyb Oct 7, 2024

hellkite500 Sep 12, 2024

hellkite500 Sep 12, 2024

hellkite500 Sep 12, 2024

hellkite500 Sep 12, 2024

hellkite500 Sep 12, 2024

taddyb Oct 7, 2024

taddyb commented Oct 1, 2024

taddyb commented Oct 2, 2024 •

edited

Loading

taddyb commented Oct 8, 2024

taddyb commented Oct 9, 2024 •

edited

Loading

kumdonoaa commented Oct 9, 2024

taddyb commented Oct 9, 2024

taddyb commented Oct 21, 2024 •

edited

Loading

kumdonoaa commented Nov 4, 2024

taddyb commented Nov 5, 2024


		To use these files, follow the steps below:

		1. Copy the `test_compose.yaml` file in the root project dir


		RUN uv pip install --no-cache-dir -r requirements.txt

		ENV WITH_EDITABLE=true


		RUN uv pip install --no-cache-dir -r requirements-app.txt

		WORKDIR "/t-route/src/"

		@@ -0,0 +1,18 @@
		#!/bin/bash

		if ! docker login --username ${GH_USERNAME} --password ${GH_TOKEN} ghcr.io; then

RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837

Are you sure you want to change the base?

RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837

Conversation

taddyb33 commented Sep 3, 2024 • edited Loading

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Testing checklist

Target Environment support

Accessibility

Other

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taddyb33 commented Sep 9, 2024

AminTorabi-NOAA commented Sep 9, 2024

taddyb33 commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taddyb commented Oct 1, 2024

taddyb commented Oct 2, 2024 • edited Loading

taddyb commented Oct 8, 2024

taddyb commented Oct 9, 2024 • edited Loading

kumdonoaa commented Oct 9, 2024

taddyb commented Oct 9, 2024

taddyb commented Oct 21, 2024 • edited Loading

kumdonoaa commented Nov 4, 2024

taddyb commented Nov 5, 2024

taddyb33 commented Sep 3, 2024 •

edited

Loading

taddyb commented Oct 2, 2024 •

edited

Loading

taddyb commented Oct 9, 2024 •

edited

Loading

taddyb commented Oct 21, 2024 •

edited

Loading