improve multi_timeseries call#150
Conversation
Enovotny
commented
Jun 6, 2025
Enovotny
commented
- update to improve multi_timeseries call to use concurrent.futures instead of threading.
- Added retries for all API calls if connection fails. Retries set to 6.
…lls in connection fails.
| begin: Optional[datetime] = None, | ||
| end: Optional[datetime] = None, | ||
| melted: Optional[bool] = False, | ||
| max_workers: Optional[int] = 30, |
There was a problem hiding this comment.
You have 30 here but below in the doc you suggest it would be 32 from 3.8 on, maybe change this to 32 to match that?
| ts_ids: linst | ||
| a list of timeseries to get. If the timeseries is a verioned timeseries then serpeate the ts_id from the | ||
| ts_ids: list | ||
| a list of timeseries to get. If the timeseries is a versioned timeseries then separate the ts_id from the |
There was a problem hiding this comment.
I would consider breaking your commits by task, or 1 commit for renaming (spell checking) and another for the multi TS stuff for example.
I am not suggesting a new PR, just separate commits will help reviewers per task! Just my opinion on what an atomic commit could be.
| } | ||
| return result_dict | ||
|
|
||
| with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: |
There was a problem hiding this comment.
I'm not sure what kind of error handling we have, but when users slam the API with threaded calls there's a chance the API will not respond. Then those calls get retried the 6 times.
I would go about doing some research into the various different ways a futures call could fail and possible add those into our Error handling so it's a little more understanding for the end user what went wrong.
Another possibility is one of these calls fails in some way in the get_ts_ids function and it could potentially cause them all to fail if they have shared state.
There's a few ways you could handle this but most the time it wouldn't fail..
There was a problem hiding this comment.
I just realized you could randomly throw an error to actually test what the threading would do
i.e.
raise Exception("Test!")
Inside the get_ts_ids function - would the rest finish?
And see what happens
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Looks good!
None of these changes are blocking in my opinion! Just suggestions that if you wanted to do something about you could.
|
2 participants
