-
Notifications
You must be signed in to change notification settings - Fork 50
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
RTX Code Release (PI-3): Enhancements to T-Route so it can be used for OWP's Replace and Route #837
base: master
Are you sure you want to change the base?
Conversation
migrated fastapi wrapper to the troute branch
…into migration
Migration
…into migration
Documentation
added test files
test/api/README.md
Outdated
|
||
To use these files, follow the steps below: | ||
|
||
1. Copy the `test_compose.yaml` file in the root project dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thetest/api/data
is located. This was to prevent having to change the mounted volume location within thetest_compose.yaml
file - You'll notice in the instructions I added (see below) the
feature_id
andhy_id
are the same. I put this this way because of how RnR calls this API. RnR saves it's geopackages based on theirfeature_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 theLowerColorado
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
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 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 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.
|
The non-editable flag: ./compiler.sh no-e is no longer necessary (see NOAA-OWP#675 for details)
Removing lower colorado duplicated files
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:
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.
That data format is related to how I coded replace and route's file directory.
The way that the API works is we use a |
.gitignore
Outdated
data/ | ||
output | ||
test_compose.yaml | ||
src/app/.ruff_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line for posix
Dockerfile.troute_api
Outdated
WORKDIR "/t-route/" | ||
COPY . "/t-route/" | ||
|
||
RUN ln -s /usr/lib64/gfortran/modules/netcdf.mod /usr/include/openmpi-x86_64/netcdf.mod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the ENV var to point to the include dir
https://github.com/NOAA-OWP/t-route/blob/470c767428f130d79743feed0d792cceafbdcd6d/compiler.sh#L45C15-L45C32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Dockerfile.troute_api
Outdated
|
||
RUN uv pip install --no-cache-dir -r requirements.txt | ||
|
||
ENV WITH_EDITABLE=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think an editable build is really needed here
Dockerfile.troute_api
Outdated
|
||
RUN uv pip install --no-cache-dir -r requirements-app.txt | ||
|
||
WORKDIR "/t-route/src/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
posix newline
@@ -0,0 +1,18 @@ | |||
#!/bin/bash | |||
|
|||
if ! docker login --username ${GH_USERNAME} --password ${GH_TOKEN} ghcr.io; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/app/core/base_config.yaml
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More cowbell!
src/app/schemas.py
Outdated
from pydantic import BaseModel | ||
|
||
|
||
class TRouteOuput(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -0,0 +1 @@ | |||
from .__main__ import main_v04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More Cowbell!
test/api/test_compose.yaml
Outdated
interval: 30s | ||
timeout: 5s | ||
retries: 3 | ||
start_period: 5s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More cowbell!
test/api/README.md
Outdated
@@ -0,0 +1,22 @@ | |||
# Testing the T-Route FastAPI Extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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:
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 The endpoint is No changes to the code, or the paths of the files are required |
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 I also moved all compose.yaml files into This should make testing, and running the API, easier. Lastly, docs have been updated in edit: changed the reference from |
@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.
|
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