Skip to content

Add a TS Hook for Fetching Multiple TimeSeries at Once#138

Open
krowvin wants to merge 17 commits intomainfrom
multi-ts-hook
Open

Add a TS Hook for Fetching Multiple TimeSeries at Once#138
krowvin wants to merge 17 commits intomainfrom
multi-ts-hook

Conversation

@krowvin
Copy link
Contributor

@krowvin krowvin commented Jul 22, 2025

In preparation for the CWMSTable and CWMSPlot hook updates I wanted to create a multi-ts hook.

Instead of merging this into the existing useCdaTimeSeries I thought I would go the route of cwms-python and create a separate call for doing multi-ts.

Some talking points:

  • When do we want to get clever with the cache? I.e. better with caching dates of a timeseries
  • I'm running a promise.all inside 1 useQuery. I was not sure how to make multiple useQuery. This might be better in terms of each timeseries would have it's own state? Memory issues?
  • Does it make sense to use , for the timeseries delimiter? V1 of the API used | or perhaps we use an Array of strings?

@krowvin krowvin requested a review from jbkolze July 22, 2025 20:31
@krowvin krowvin added the enhancement New feature or request label Jul 22, 2025
@MikeNeilson
Copy link

FYI the v1 multi time series by | separate string will be going away at some point. You should favor doing parallel queries and doing some form of join or yield operation.

@krowvin
Copy link
Contributor Author

krowvin commented Jul 22, 2025

You should favor doing parallel queries

This is a parallel query and does not use v1. It splits on , and loops that array triggering cwmsjs to build the query urls and stores them into one state for re-use. (Larger state convo here)

My mention of the v1 was more about it's choice of | instead of , as a delimiter

Sorry for the confusion.

https://github.com/USACE-WaterManagement/groundwork-water/pull/138/files#diff-44134e2578417fcea9bc82b6bf059595dd3ec5984ba7afae9ebfa6805354cf34R22

Copy link
Collaborator

@jbkolze jbkolze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed externally, I think it might be a little smoother to use useQueries here instead of useQuery. That would replace the Promise.all and return more granular results to the user.

@krowvin krowvin requested a review from jbkolze August 20, 2025 04:14
Copy link
Collaborator

@jbkolze jbkolze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general. Recommended removing type assertions and checking example TSIDs. Other stuff is at your discretion.

Comment on lines 29 to 30
"KEYS.Elev.Inst.1Hour.0.Ccp-Rev",
"KEYS.Stor.Inst.1Hour.0.Ccp-Rev",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These TSIDs are throwing errors for me. Works as expected when I change to working ones, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting issues with this as well. They would work off and on against the national CDA.
Do you see data via this URL?
https://cwms-data.usace.army.mil/cwms-data/timeseries?name=KEYS.Elev.Inst.1Hour.0.Ccp-Rev&office=SWT

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that throws a system error for me -- same thing i was seeing in the docs:

{
"message": "System Error",
"incidentIdentifier": "1109935419268345744",
"details": {}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, while further testing the docs they suddenly started populating? Not sure -- seems like might be something odd going on with these records.

cwmsJsType="GetTimeSeriesRequest"
/>
<div className="font-bold text-lg pt-6">cdaParams</div>
<ParamsTable paramsList={cdaTSHookParams} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default ParamsTable doesn't indicate that "name" should be passed as an array (or comma-separated string).

This actually somewhat raises the question of whether the data should be mapped in this manner, or whether each time series should have its own set of parameters? With the current setup a provided "unit" value would get applied to every provided TSID, which might cause issues if you're pulling both flow and elevation, for example.

Might be a "future enhancement" thing.

Copy link
Contributor Author

@krowvin krowvin Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a "future enhancement" thing.

I had the same thought, but wondered if it was a goose chase to pursue it now. But since you said something and i didn't realize it would break unit I'd lean towards all in and go ahead and just make it an array of the CDATSParam object.

Edit: i was thinking of begin/end times when I thought of it. I.e. all the TS would have to have the same start/stop dates. Which can make caching weird too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good as-is, but if someone gets antsy in the future we could potentially allow for setting "default" params and allowing custom overrides for individual TSIDs as required. Because I do think typically the begin/end dates will be consistent, like you mentioned.

@krowvin
Copy link
Contributor Author

krowvin commented Aug 20, 2025

Alright, check those out. See what you think. For some reason the GW table overflow as not working for me despite me updating GW to the latest. I'm wondering if it was a cache issue? Tried nuking my node_modules too.

I also debated if the TSMulti hook should return an object/map of these TS Param objects keyed to the TSID. But that could go wrong considering multiple offices can have the same tsid name! So sticking with an array that contains all the responses from CDA for now.

@krowvin
Copy link
Contributor Author

krowvin commented Aug 20, 2025

Had a few conflicts there. Mainly with the package lock. Probably should have pulled main in first before trying to bump the version. For now I tried to revert back to the last package.json/package lock.

Will bump groundwork in a sep PR I think! (Was mostly for table overflow)

@krowvin krowvin requested a review from jbkolze August 20, 2025 22:08
Copy link
Collaborator

@jbkolze jbkolze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're playing formatter ping-pong again. Looks like the husky commit hooks aren't working on your end for some reason? We will have to get together to figure that out. And maybe an indication that it would be helpful to add a prettier -check to the merge GH Action.

In the meantime, can you just do an npx prettier -w . from the root when you're done with any other changes? That should take care of it.

}
// Stable API client across renders unless config changes
const timeseriesApi = useMemo(() => new TimeSeriesApi(config), [config]);
const TIME_SERIES_API = useMemo(() => new TimeSeriesApi(config), [config]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like using FULL_UPPERCASE naming is a little misleading here since this isn't really a run-time constant?

const baseKey = useMemo(() => {
const { name, ...rest } = cdaParams || {};
return ["cda", "timeseries", JSON.stringify(rest)];
const TIME_SERIES_IDS = useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above with the FULL_UPPERCASE naming. Would probably just stick to camelCase here.

Comment on lines 29 to 30
"KEYS.Elev.Inst.1Hour.0.Ccp-Rev",
"KEYS.Stor.Inst.1Hour.0.Ccp-Rev",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, while further testing the docs they suddenly started populating? Not sure -- seems like might be something odd going on with these records.

cwmsJsType="GetTimeSeriesRequest"
/>
<div className="font-bold text-lg pt-6">cdaParams</div>
<ParamsTable paramsList={cdaTSHookParams} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good as-is, but if someone gets antsy in the future we could potentially allow for setting "default" params and allowing custom overrides for individual TSIDs as required. Because I do think typically the begin/end dates will be consistent, like you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants