Conversation
| # If getting extents fails, fall back to single-threaded mode | ||
| print( | ||
| f"WARNING: Could not retrieve time series extents ({e}). Falling back to single-threaded mode." | ||
| logging.debug( |
There was a problem hiding this comment.
I would actually call this an error instead of debug because it means the timeseries extents is failing for some reason.
There was a problem hiding this comment.
could be an error, but the request still goes through. it just doesn't use the multithread. so I don't want an error message to appear if the request completes. If we killed the entire request then yes we would put error.
|
|
||
| # Assert the expected values | ||
| assert chunks == 5, f"Expected 5 chunks, but got {chunks}" | ||
| assert threads == 5, f"Expected 5 threads, but got {threads}" |
There was a problem hiding this comment.
Do we want to still test for the number of threads or call it good since we've tested it already?
There was a problem hiding this comment.
I don't think a logging message should be used for a test. It might be a test of the individual function it self that chunks the data. run a test for chunk_timeseries_data. to make sure that it provides the data chunked correctly. actually that should be what we do...
There was a problem hiding this comment.
Makes sense. Maybe the actual threads should be a separate function too and tested.
|
The multi-threading changes looks good, just a couple comments. I didn't look close at the api changes. |
perrymanmd
left a comment
perrymanmd
left a comment
There was a problem hiding this comment.
Just cleaning up some change from print to logging.
cwms/timeseries/timeseries.py
Outdated
| chunk_start, chunk_end = future_to_chunk[future] | ||
| print( | ||
| logging.error( | ||
| f"ERROR: Failed to fetch data from {chunk_start} to {chunk_end}: {e}" |
There was a problem hiding this comment.
Remove "ERROR: " and let the logging level handle it.
cwms/timeseries/timeseries.py
Outdated
| actual_workers = min(max_workers, len(chunks)) | ||
| print( | ||
| logging.debug( | ||
| f"INFO: Storing {len(chunks)} chunks of timeseries data with {actual_workers} threads" |
There was a problem hiding this comment.
Remove "INFO: " and let the logging level handle it.
…meseries-post-return
…st-return' of https://github.com/HydrologicEngineeringCenter/cwms-python into 202-apipy-errors-when-decoding-successful-timeseries-post-return
|
3 participants
fix message from posting
change print statements from timeseries store/get to logging