Stabilize NWIS Tests and Improve 5xx Error Handling#223
Stabilize NWIS Tests and Improve 5xx Error Handling#223thodson-usgs wants to merge 8 commits intoDOI-USGS:mainfrom
Conversation
thodson-usgs
changed the title
There was a problem hiding this comment.
Pull request overview
This PR continues the NWIS deprecation work by ensuring defunct NWIS entry points fail in a controlled way, updating tests to reflect the deprecations (and reducing reliance on live NWIS responses for some cases), and improving handling of transient NWIS 5xx/HTML error responses.
Changes:
- Updated
tests/nwis_test.pyto use mocked JSON fixtures for IV parsing/empty responses and to assert defunct NWIS functions/services raise errors. - Added JSON fixtures to support mocked NWIS IV responses in tests.
- Improved
utils.query()handling for 5xx responses and added HTML-instead-of-JSON detection inget_dv()/get_iv().
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/nwis_test.py | Switches some IV tests to mocked responses, adds defunct-service assertions, and adds a new live NWIS sanity check. |
| tests/data/nwis_iv_mock.json | Adds a minimal JSON response fixture for IV parsing tests. |
| tests/data/nwis_iv_empty_mock.json | Adds an empty timeSeries JSON fixture for empty-response behavior. |
| dataretrieval/utils.py | Raises a clearer ValueError for HTTP 500/502/503 “service unavailable” responses. |
| dataretrieval/nwis.py | Adds HTML-response detection for get_dv()/get_iv(), updates get_record docs/service validation, and deprecates NWIS_Metadata.variable_info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_iv_service_answer(requests_mock): | ||
| df = _test_iv_service(requests_mock) | ||
| # check multiindex function | ||
| assert df.index.names == [ | ||
| SITENO_COL, | ||
| DATETIME_COL, | ||
| ], f"iv service returned incorrect index: {df.index.names}" | ||
|
|
There was a problem hiding this comment.
test_iv_service_answer now requires the requests_mock fixture, but this file still has a __main__ block that calls it directly (without providing the fixture). Running the module directly will therefore crash; consider removing the __main__ block or updating it to call a non-fixture helper instead.
dataretrieval/nwis.py
Outdated
| "Received HTML response instead of JSON. This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise e |
There was a problem hiding this comment.
In the JSON parsing error handling, raise e resets the traceback context. Using bare raise inside the except will preserve the original traceback, making failures easier to diagnose while keeping behavior the same.
| raise e | |
| raise |
dataretrieval/nwis.py
Outdated
| "Received HTML response instead of JSON. This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise e |
There was a problem hiding this comment.
Same as above: raise e here will overwrite the traceback. Prefer bare raise to preserve the original exception context when you’re not wrapping it.
| raise e | |
| raise |
dataretrieval/nwis.py
Outdated
| >>> # Get site description for site 01585200 | ||
| >>> df = dataretrieval.nwis.get_record(sites="01585200", service="site") | ||
|
|
There was a problem hiding this comment.
The get_record docstring examples contain a duplicated “Get site description…” snippet, which looks like an accidental copy/paste and makes the examples harder to follow. Consider removing the duplicate and (if needed) replacing it with an example relevant to the remaining supported services.
| >>> # Get site description for site 01585200 | |
| >>> df = dataretrieval.nwis.get_record(sites="01585200", service="site") |
dataretrieval/nwis.py
Outdated
| - 'water_use': get water use data | ||
| - 'gwlevels': (defunct) use `waterdata.get_field_measurements` | ||
| - 'pmcodes': (defunct) use `get_reference_table` | ||
| - 'water_use': (defunct) defunct |
There was a problem hiding this comment.
The service list entry for water_use reads “(defunct) defunct”, which is redundant and doesn’t clearly communicate whether there is a replacement. Consider rewording to something like “(defunct) no replacement available” (matching the get_water_use behavior) for clarity.
| - 'water_use': (defunct) defunct | |
| - 'water_use': (defunct) no replacement available |
dataretrieval/nwis.py
Outdated
| @@ -1235,4 +1235,10 @@ def site_info(self) -> tuple[pd.DataFrame, BaseMetadata] | None: | |||
| def variable_info(self) -> tuple[pd.DataFrame, BaseMetadata] | None: | |||
| # define variable_info metadata based on parameterCd if available | |||
| if "parameterCd" in self._parameters: | |||
| return get_pmcodes(parameterCd=self._parameters["parameterCd"]) | |||
| warnings.warn( | |||
| "Accessing variable_info via NWIS_Metadata is deprecated as " | |||
| "it relies on the defunct get_pmcodes function.", | |||
| DeprecationWarning, | |||
| stacklevel=2, | |||
| ) | |||
| return None | |||
There was a problem hiding this comment.
NWIS_Metadata.variable_info now always returns None (with a DeprecationWarning) even when parameterCd is present. The class docstring/type description above still suggests variable info will be available when parameterCd is provided; consider updating the documentation/type notes to reflect the new deprecated behavior so callers aren’t misled.
tests/nwis_test.py
Outdated
| """Live sanity check of NWIS service, tolerant of 502/503.""" | ||
| site = "01491000" | ||
| try: | ||
| # Minimal query: just most recent record | ||
| get_iv(sites=site) | ||
| except ValueError as e: | ||
| # Catch our custom 5xx error from utils.py | ||
| if any(err in str(e) for err in ["502", "503", "Service Unavailable"]): | ||
| pytest.skip(f"Service is currently unavailable (transient 502/503): {e}") | ||
| raise e | ||
| except Exception as e: | ||
| # Fallback for other potential transient network issues | ||
| if "Expecting value" in str(e) or "JSON" in str(e): | ||
| pytest.skip(f"Service returned invalid response (likely 502/503): {e}") |
There was a problem hiding this comment.
test_nwis_service_live is meant to be tolerant of transient NWIS outages, but the exception handling will currently re-raise some of the new “service unavailable” failures:
utils.query()now raisesValueErrorfor HTTP 500 as well as 502/503, but the test only skips 502/503.get_iv()can now raise aValueErrorlike “Received HTML response instead of JSON…”. Because it’s aValueError, it’s caught by the firstexcept ValueErrorblock and then re-raised (it won’t reach the fallbackexcept Exceptionthat checks for “JSON”).
Consider broadening the skip condition in theValueErrorhandler (e.g., also skip on 500 and on the HTML-instead-of-JSON message, or generally on either of these known transient ValueErrors).
| """Live sanity check of NWIS service, tolerant of 502/503.""" | |
| site = "01491000" | |
| try: | |
| # Minimal query: just most recent record | |
| get_iv(sites=site) | |
| except ValueError as e: | |
| # Catch our custom 5xx error from utils.py | |
| if any(err in str(e) for err in ["502", "503", "Service Unavailable"]): | |
| pytest.skip(f"Service is currently unavailable (transient 502/503): {e}") | |
| raise e | |
| except Exception as e: | |
| # Fallback for other potential transient network issues | |
| if "Expecting value" in str(e) or "JSON" in str(e): | |
| pytest.skip(f"Service returned invalid response (likely 502/503): {e}") | |
| """Live sanity check of NWIS service, tolerant of transient NWIS outages.""" | |
| site = "01491000" | |
| try: | |
| # Minimal query: just most recent record | |
| get_iv(sites=site) | |
| except ValueError as e: | |
| # Catch known transient service failures surfaced as ValueError | |
| error_text = str(e) | |
| if any( | |
| err in error_text | |
| for err in [ | |
| "500", | |
| "502", | |
| "503", | |
| "Service Unavailable", | |
| "Received HTML response instead of JSON", | |
| ] | |
| ): | |
| pytest.skip(f"Service is currently unavailable (transient NWIS outage): {e}") | |
| raise e | |
| except Exception as e: | |
| # Fallback for other potential transient network issues | |
| if "Expecting value" in str(e) or "JSON" in str(e): | |
| pytest.skip(f"Service returned invalid response (likely transient outage): {e}") |
| try: | ||
| df = _read_json(response.json()) | ||
| except Exception as e: | ||
| if "<html>" in response.text.lower(): | ||
| raise ValueError( | ||
| "Received HTML response instead of JSON. This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e |
There was a problem hiding this comment.
The HTML-detection heuristic only checks for "<html>" in the response text. NWIS error pages may start with <!DOCTYPE html> (which you already check for in _read_rdb); consider also checking for "<!doctype" and/or using the Content-Type header to make this more robust.
|
@copilot apply changes based on the comments in this thread |
…ction, and test refinement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_iv_service_answer(requests_mock): | ||
| df = _test_iv_service(requests_mock) | ||
| # check multiindex function | ||
| assert df.index.names == [ | ||
| SITENO_COL, | ||
| DATETIME_COL, | ||
| ], f"iv service returned incorrect index: {df.index.names}" | ||
|
|
||
| """Live sanity check of NWIS service, tolerant of transient NWIS outages.""" | ||
| site = "01491000" | ||
| try: | ||
| # Minimal query: just most recent record | ||
| get_iv(sites=site) |
There was a problem hiding this comment.
test_iv_service_answer mixes a mocked unit test with a live network call to get_iv(), and includes a stray triple-quoted string literal inside the function body. Because this test uses the requests_mock fixture, any unregistered HTTP call is typically blocked/raises, so the live call is likely to fail in CI (or become nondeterministic). Split the live call into a separately marked integration test that does not use requests_mock (or remove it), and convert the in-body docstring to a regular comment.
| def _load_mock_json(file_name): | ||
| """Helper to load mock JSON from tests/data.""" | ||
| with open(f"tests/data/{file_name}") as f: | ||
| return json.load(f) |
There was a problem hiding this comment.
_load_mock_json opens tests/data/... using a path relative to the current working directory, which can fail when tests are executed from a different CWD (e.g., tox, editable installs). Resolve the file path relative to __file__ (e.g., Path(__file__).parent / "data" / file_name) and consider specifying an explicit encoding.
tests/nwis_test.py
Outdated
| mock_url = ( | ||
| "https://waterservices.usgs.gov/nwis/iv?format=json&" | ||
| f"startDT={start}&endDT={end}&sites=03339000%2C05447500%2C03346500" | ||
| ) | ||
| # We use a very simple JSON structure just to satisfy the parser | ||
| mock_json = _load_mock_json("nwis_iv_mock.json") | ||
|
|
||
| requests_mock.get(mock_url, json=mock_json) |
There was a problem hiding this comment.
The request mock registers a single fully-specified URL string. This is brittle because the production request is built via requests.get(..., params=...), so query param ordering/extra params can change and break the mock unexpectedly. Prefer matching the base endpoint URL and asserting/query-matching parameters via the mocking library’s querystring matching facilities (or register with complete_qs=False explicitly).
| mock_url = ( | |
| "https://waterservices.usgs.gov/nwis/iv?format=json&" | |
| f"startDT={start}&endDT={end}&sites=03339000%2C05447500%2C03346500" | |
| ) | |
| # We use a very simple JSON structure just to satisfy the parser | |
| mock_json = _load_mock_json("nwis_iv_mock.json") | |
| requests_mock.get(mock_url, json=mock_json) | |
| mock_url = "https://waterservices.usgs.gov/nwis/iv" | |
| expected_sites = ",".join(site) | |
| def _match_query(request): | |
| return request.qs == { | |
| "format": ["json"], | |
| "startDT": [start], | |
| "endDT": [end], | |
| "sites": [expected_sites], | |
| } | |
| # We use a very simple JSON structure just to satisfy the parser | |
| mock_json = _load_mock_json("nwis_iv_mock.json") | |
| requests_mock.get(mock_url, json=mock_json, additional_matcher=_match_query) |
| defunct_services = ["measurements", "gwlevels", "pmcodes", "water_use"] | ||
| if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES + defunct_services: | ||
| raise TypeError(f"Unrecognized service: {service}") | ||
|
|
There was a problem hiding this comment.
get_record now treats defunct services as “recognized” via defunct_services, but the implementation below still dispatches into the old branches. At least measurements and gwlevels will raise TypeError due to mismatched keyword arguments (site_no/begin_date/startDT/endDT) before the defunct functions can raise the intended NameError. Handle defunct services explicitly in get_record (raise the same NameError guidance directly) or update the dispatch call signatures to match the defunct wrapper signatures.
| if service in defunct_services: | |
| raise NameError( | |
| f"The NWIS service '{service}' is no longer supported by get_record." | |
| ) |
| try: | ||
| df = _read_json(response.json()) | ||
| except Exception as e: | ||
| if ( | ||
| "<html>" in response.text.lower() | ||
| or "<!doctype" in response.text.lower() | ||
| or "text/html" in response.headers.get("Content-Type", "").lower() | ||
| ): | ||
| raise ValueError( | ||
| "Received HTML response instead of JSON. This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
The new HTML-detection error wraps any exception from response.json()/_read_json, which can mask genuine parsing/logic bugs unrelated to HTML responses. Consider catching only JSON decode errors (or ValueError) and include response.status_code and response.url in the raised ValueError to aid debugging when CI fails.
dataretrieval/nwis.py
Outdated
| def variable_info(self) -> None: | ||
| """ | ||
| Deprecated. Accessing variable_info via NWIS_Metadata is deprecated | ||
| as it relied on the defunct `get_pmcodes` function. Returns None. | ||
| """ | ||
| # define variable_info metadata based on parameterCd if available | ||
| if "parameterCd" in self._parameters: | ||
| return get_pmcodes(parameterCd=self._parameters["parameterCd"]) | ||
| warnings.warn( | ||
| "Accessing variable_info via NWIS_Metadata is deprecated as " | ||
| "it relies on the defunct get_pmcodes function.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
NWIS_Metadata.variable_info is now annotated as returning None, but the class docstring above still documents variable_info as returning a (DataFrame, Metadata) | None tuple. Also, when parameterCd is not present there is no explicit return, which is fine at runtime but makes the intent less clear. Update the class-level documentation/type hints to reflect the deprecation and return value, and consider warning on any access to variable_info (not only when parameterCd exists) for consistent deprecation behavior.
| try: | ||
| df = _read_json(response.json()) | ||
| except Exception as e: | ||
| if ( | ||
| "<html>" in response.text.lower() | ||
| or "<!doctype" in response.text.lower() | ||
| or "text/html" in response.headers.get("Content-Type", "").lower() | ||
| ): | ||
| raise ValueError( | ||
| "Received HTML response instead of JSON. This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
Same as get_dv: this exception handler catches any exception from response.json()/_read_json and only special-cases HTML responses. To avoid masking non-HTML parsing bugs, consider narrowing the caught exception types and include response.status_code and response.url in the raised error for easier diagnosis.
tests/nwis_test.py
Outdated
| mock_url = ( | ||
| f"https://waterservices.usgs.gov/nwis/iv?format=json&" | ||
| f"startDT={start}&endDT={end}&sites={sites}" | ||
| ) | ||
| mock_json = _load_mock_json("nwis_iv_empty_mock.json") | ||
| requests_mock.get(mock_url, json=mock_json) | ||
|
|
There was a problem hiding this comment.
This mock also registers a fully-specified URL string with query parameters, which can become fragile if the underlying request adds/reorders params (since requests builds the URL from params). Prefer registering the base endpoint and matching/asserting query parameters via the mock library to reduce flakiness.
…ndling, and defunct service logic
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_nwis_service_live(): | ||
| """Live sanity check of NWIS service, tolerant of transient NWIS outages.""" | ||
| site = "01491000" | ||
| try: | ||
| # Minimal query: just most recent record | ||
| get_iv(sites=site) | ||
| except ValueError as e: | ||
| # Catch known transient service failures surfaced as ValueError | ||
| error_text = str(e) | ||
| if any( | ||
| err in error_text | ||
| for err in [ | ||
| "500", | ||
| "502", | ||
| "503", | ||
| "Service Unavailable", | ||
| "Received HTML response instead of JSON", | ||
| ] | ||
| ): | ||
| pytest.skip( | ||
| f"Service is currently unavailable (transient NWIS outage): {e}" | ||
| ) | ||
| raise | ||
| except Exception as e: | ||
| # Fallback for other potential transient network issues | ||
| if "Expecting value" in str(e) or "JSON" in str(e): | ||
| pytest.skip( | ||
| f"Service returned invalid response (likely transient outage): {e}" | ||
| ) | ||
| raise |
There was a problem hiding this comment.
test_nwis_service_live performs a real network call during the normal test run. This will still fail in CI/environments without outbound access (e.g., ConnectionError/timeout), and the current exception handling doesn’t skip those cases. Consider marking this as an integration/network test and skipping by default (e.g., via a pytest marker or env var gate), or convert it to a requests-mock based test.
| response = query_waterservices("dv", format="json", ssl_check=ssl_check, **kwargs) | ||
| df = _read_json(response.json()) | ||
| try: | ||
| df = _read_json(response.json()) | ||
| except (ValueError, requests.exceptions.JSONDecodeError) as e: | ||
| if ( | ||
| "<html>" in response.text.lower() | ||
| or "<!doctype" in response.text.lower() | ||
| or "text/html" in response.headers.get("Content-Type", "").lower() | ||
| ): | ||
| raise ValueError( | ||
| f"Received HTML response instead of JSON from {response.url} " | ||
| f"(Status: {response.status_code}). This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
Catching requests.exceptions.JSONDecodeError is a compatibility risk because requests has no minimum version pinned; in versions where this attribute is missing, the except (...) clause can raise AttributeError when a JSON parse error occurs. Since response.json() already raises a ValueError/json.JSONDecodeError, consider catching ValueError (or json.JSONDecodeError) only, or importing JSONDecodeError from json for a stable exception type.
| response = query_waterservices( | ||
| service="iv", format="json", ssl_check=ssl_check, **kwargs | ||
| ) | ||
|
|
||
| df = _read_json(response.json()) | ||
| try: | ||
| df = _read_json(response.json()) | ||
| except (ValueError, requests.exceptions.JSONDecodeError) as e: | ||
| if ( | ||
| "<html>" in response.text.lower() | ||
| or "<!doctype" in response.text.lower() | ||
| or "text/html" in response.headers.get("Content-Type", "").lower() | ||
| ): | ||
| raise ValueError( | ||
| f"Received HTML response instead of JSON from {response.url} " | ||
| f"(Status: {response.status_code}). This often indicates " | ||
| "that the service is currently unavailable." | ||
| ) from e | ||
| raise |
There was a problem hiding this comment.
Same as above: the except (ValueError, requests.exceptions.JSONDecodeError) clause risks AttributeError on older/unpinned requests versions. Prefer catching ValueError and/or json.JSONDecodeError (stdlib) to keep JSON error handling stable across dependency versions.
| if service in defunct_services: | ||
| raise NameError( | ||
| f"The NWIS service '{service}' is no longer supported by get_record." | ||
| ) | ||
|
|
||
| if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES + defunct_services: |
There was a problem hiding this comment.
After raising for defunct_services, the later elif service == "measurements"/"gwlevels"/"pmcodes"/"water_use" branches become unreachable dead code. Consider removing those branches (or restructuring the conditional) to avoid confusion and ensure the implementation matches the documented/validated service behavior.
| if service in defunct_services: | |
| raise NameError( | |
| f"The NWIS service '{service}' is no longer supported by get_record." | |
| ) | |
| if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES + defunct_services: | |
| supported_services = WATERSERVICES_SERVICES + WATERDATA_SERVICES | |
| if service not in supported_services + defunct_services: |
| def variable_info(self) -> None: | ||
| """ | ||
| Deprecated. Accessing variable_info via NWIS_Metadata is deprecated. | ||
| Returns None. | ||
| """ | ||
| warnings.warn( | ||
| "Accessing variable_info via NWIS_Metadata is deprecated as " | ||
| "it relies on the defunct get_pmcodes function.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
NWIS_Metadata.variable_info now always returns None and emits a DeprecationWarning, but the class docstring/type description above still describes variable_info as returning a (DataFrame, Metadata) | None. Please update the docstring/typing to reflect the new behavior, and consider adding a test that asserts the warning is raised (and None is returned) so this deprecation behavior stays stable.
| elif response.status_code in [500, 502, 503]: | ||
| raise ValueError( | ||
| f"Service Unavailable: {response.status_code} {response.reason}. " | ||
| + f"The service at {response.url} may be down or experiencing issues." | ||
| ) |
There was a problem hiding this comment.
utils.query() now special-cases 500/502/503 responses, but there’s no corresponding unit test coverage to ensure these codes reliably raise the intended ValueError (and that other codes keep their existing behavior). Since tests/utils_test.py already covers query(), consider adding a requests-mock based test for at least one 5xx status code here.
This PR addresses several recent CI failures by stabilizing the NWIS test suite and improving HTTP error handling.
Key Changes:
utils.pyto better handle 500, 502, and 503 HTTP status codes from USGS services, which have been causing transient CI failures.NWIS_Metadata.variable_infoandget_recordto ensure consistent and informative behavior during service deprecation._walk_pagesinwaterdata/utils.pyto use list-based aggregation, reducing memory copying overhead from_get_argshelper and refactored all 11 API functions inwaterdata/api.pyto use it.to_strinutils.pywithmap(str, ...)and broader iterable support (sets, tuples, generators).waterdata_utils_test.pyand expandedtests/utils_test.py.