From dc0da2f7755c6ea5c17da77efe560e48a4b7fbeb Mon Sep 17 00:00:00 2001 From: zshamroukh Date: Tue, 4 Aug 2026 16:16:22 -0400 Subject: [PATCH 1/5] fix: make property_v2 `limit` a total cap and warn on truncation (DAT-121) Passing any explicit `limit` to property_v2.search.retrieve silently disabled auto-pagination, so `limit=1000` returned one page of 1,000 properties and discarded every remaining match with no error and no warning. A customer built a 50-CBSA research panel this way and received ~0.4% of the data in a DataFrame that looked complete. `limit` is now a cap on the total number of properties returned, with pagination handled internally to satisfy it. Calls with `limit <= 50000` are unaffected -- same request, same results -- so the behavioural fix here is that partial results now announce themselves. - `limit` above 50,000 paginates instead of failing the request (the schema's `le` bound rejected these before reaching the pagination logic at all) - ParclLabsTruncationWarning when `limit` withholds matching data, reporting returned vs available counts; once per session so per-market loops stay usable - failed pages are retried with exponential backoff, then reported via ParclLabsIncompleteResultWarning and metadata["incomplete_pages"], instead of being printed and skipped as if the result were complete - pagination integrity check warns if assembled pages do not yield the expected number of distinct properties - stop leaking the internal `auto_paginate` flag into the request query string - stop duplicating `limit` in paginated page URLs - `_get_metadata` deep-copies, so it no longer mutates the caller's raw response The multi-page path previously had no test coverage: `test_fetch_post_pagination` built two mock pages but called `_fetch_post` with pagination disabled and asserted a single request, so the second page was never consumed. Two further tests asserted the buggy behaviour as correct. All three are rewritten. Verified against prod (NYC parcl_id 2900187, Jan 2024, 108,295 available): limit=1000 -> 1,000 properties + truncation warning limit=60000 -> 60,000 properties over 2 pages (previously HTTP 422) limit omitted -> 108,295 properties, unchanged Breaking changes deferred to DAT-122. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 10 + parcllabs/__version__.py | 2 +- parcllabs/schemas/schemas.py | 14 +- parcllabs/services/properties/property_v2.py | 235 ++++++++++++----- parcllabs/warnings.py | 96 +++++++ tests/test_property_v2.py | 251 ++++++++++++++++--- 6 files changed, 518 insertions(+), 90 deletions(-) create mode 100644 parcllabs/warnings.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d188e8a..9e7430a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +### v1.18.0 +- **`property_v2.search.retrieve`: `limit` is now a cap on the total number of properties returned, not a page size.** Pagination is handled internally to satisfy it. Previously, passing *any* explicit `limit` silently disabled auto-pagination, so `limit=1000` returned one page of 1,000 and discarded every remaining match with no error or warning. Calls with `limit <= 50000` are unaffected — same request, same results. +- **`limit` above 50,000 now paginates instead of failing.** Previously the request was rejected by the API with `422 limit input should be less than or equal to 50000`. +- **Partial results now warn instead of passing silently.** When `limit` withholds matching data, a `ParclLabsTruncationWarning` reports how many properties were returned versus how many matched (emitted once per session). Note credits are charged per *property* returned, not per event, and because the returned DataFrame is event-level, `len(df)` is not bounded by `limit`. +- **Failed pages during pagination are now retried and reported.** Pages are retried up to 3 times with exponential backoff; if any still fail the result is returned with a `ParclLabsIncompleteResultWarning` and the failed offsets are listed in `metadata["incomplete_pages"]`. Previously a failed page was printed and skipped, returning short data indistinguishable from complete data. +- Added a pagination integrity check that warns if the assembled pages do not yield the expected number of distinct properties. +- New warning categories in `parcllabs.warnings` (`ParclLabsWarning`, `ParclLabsTruncationWarning`, `ParclLabsIncompleteResultWarning`) so callers can silence or escalate these via standard `warnings` filters. +- Fixed an internal `auto_paginate` flag leaking into the request query string. +- Fixed `_get_metadata` mutating the caller's raw first-page response via a shallow copy. + ### v1.17.2 - Added configurable request timeout to `ParclLabsClient`. Defaults to 10s connect / 90s read. Customizable via the `timeout` parameter on client instantiation. diff --git a/parcllabs/__version__.py b/parcllabs/__version__.py index 99f794b..4c0681d 100644 --- a/parcllabs/__version__.py +++ b/parcllabs/__version__.py @@ -1 +1 @@ -VERSION = "1.17.2" +VERSION = "1.18.0" diff --git a/parcllabs/schemas/schemas.py b/parcllabs/schemas/schemas.py index 11aca06..8512764 100644 --- a/parcllabs/schemas/schemas.py +++ b/parcllabs/schemas/schemas.py @@ -142,11 +142,21 @@ class PropertyV2RetrieveParams(BaseModel): ) # Pagination + # + # No upper bound: `limit` is a cap on the total number of properties returned, + # and values above the API's per-request ceiling + # (RequestLimits.PROPERTY_V2_MAX) are satisfied by paginating rather than + # rejected. Omit to retrieve every matching property. limit: int | None = Field( default=None, ge=1, - le=RequestLimits.PROPERTY_V2_MAX.value, - description=f"Number of results to return (max: {RequestLimits.PROPERTY_V2_MAX.value})", + description=( + "Maximum number of properties to return in total. Values above " + f"{RequestLimits.PROPERTY_V2_MAX.value} are fetched across multiple pages. " + "Omit to retrieve all matching properties. Credits are charged per property " + "returned; the returned DataFrame is event-level, so len(df) is not bounded " + "by this value." + ), ) # Additional parameters diff --git a/parcllabs/services/properties/property_v2.py b/parcllabs/services/properties/property_v2.py index 74877c6..2e6584c 100644 --- a/parcllabs/services/properties/property_v2.py +++ b/parcllabs/services/properties/property_v2.py @@ -1,3 +1,4 @@ +import copy import time from collections.abc import Mapping from concurrent.futures import ThreadPoolExecutor, as_completed @@ -10,6 +11,15 @@ from parcllabs.schemas.schemas import PropertyV2RetrieveParamCategories, PropertyV2RetrieveParams from parcllabs.services.parcllabs_service import ParclLabsService from parcllabs.services.validators import Validators +from parcllabs.warnings import ( + warn_incomplete_pages, + warn_integrity_mismatch, + warn_truncation, +) + +# Transient page failures are retried before a page is abandoned. +PAGE_FETCH_ATTEMPTS = 3 +PAGE_FETCH_BACKOFF_SECONDS = 1.0 class PropertyV2Service(ParclLabsService): @@ -26,52 +36,107 @@ def _raise_http_error(chunk_num: int, status_code: int, response_preview: str) - def _raise_empty_response_error(chunk_num: int) -> None: raise RuntimeError(f"Chunk {chunk_num} failed: Empty response from API") - def _fetch_post(self, params: dict[str, Any], data: dict[str, Any]) -> list[dict]: - """Fetch data using POST request with pagination support.""" + def _fetch_page( + self, + data: dict[str, Any], + params: dict[str, Any], + offset: int, + limit: int, + ) -> dict: + """Fetch a single page, retrying transient failures with exponential backoff. + + Raises the last exception if every attempt fails. + """ + page_params = dict(params) + page_params["limit"] = limit + page_params["offset"] = offset + + last_exc: Exception | None = None + for attempt in range(PAGE_FETCH_ATTEMPTS): + try: + return self._post(url=self.full_post_url, data=data, params=page_params).json() + except Exception as exc: # retried below, then re-raised + last_exc = exc + if attempt < PAGE_FETCH_ATTEMPTS - 1: + time.sleep(PAGE_FETCH_BACKOFF_SECONDS * (2**attempt)) + raise last_exc # type: ignore[misc] + + def _fetch_post( + self, + params: dict[str, Any], + data: dict[str, Any], + max_results: int | None = None, + ) -> list[dict]: + """Fetch data using POST, paginating until ``max_results`` is satisfied. + + Args: + params: Request params. ``params["limit"]`` is the page size. + data: POST body containing the search criteria and filters. + max_results: Maximum number of properties to return in total. ``None`` + retrieves every matching property. + + Returns: + List of raw page payloads. Failed pages are omitted and reported via a + ``ParclLabsIncompleteResultWarning``. + """ + params = dict(params) response = self._post(url=self.full_post_url, data=data, params=params) result = response.json() - all_data = [result] - if params["auto_paginate"] is False: + pagination = result.get("pagination") or {} + results_meta = (result.get("metadata") or {}).get("results") or {} + total_available = results_meta.get("total_available", 0) + retrieved = results_meta.get("returned_count", 0) + + # How many properties do we actually want? Never more than exist. + target = total_available if max_results is None else min(max_results, total_available) + + if retrieved >= target or not pagination.get("has_more"): + if target < total_available: + warn_truncation(target, total_available) return all_data - # If we need to paginate, use concurrent requests - pagination = result.get("pagination") - if pagination.get("has_more"): - print("More pages to fetch, paginating additional pages...") - - limit = pagination.get("limit") - offset = pagination.get("offset") - metadata = result.get("metadata") - total_available = metadata.get("results", {}).get("total_available", 0) - - # Calculate how many more pages we need - remaining_pages = (total_available - limit) // limit - if (total_available - limit) % limit > 0: - remaining_pages += 1 - - # Generate all the URLs we need to fetch - urls = [] - current_offset = offset + limit - for _ in range(remaining_pages): - urls.append(f"{self.full_post_url}?limit={limit}&offset={current_offset}") - current_offset += limit - - # Use ThreadPoolExecutor to make concurrent requests - with ThreadPoolExecutor(max_workers=self.client.num_workers) as executor: - future_to_url = { - executor.submit(self._post, url=url, data=data, params=params): url - for url in urls - } - - for future in as_completed(future_to_url): - try: - response = future.result() - page_result = response.json() - all_data.append(page_result) - except Exception as exc: - print(f"Request failed: {exc}") + page_size = pagination.get("limit") or params.get("limit") or retrieved + offset = pagination.get("offset", 0) + + # Request exactly the pages needed to reach `target` -- the final page is + # trimmed so an explicit `limit` is honoured precisely rather than overshot. + pages: list[tuple[int, int]] = [] + current_offset = offset + retrieved + remaining = target - retrieved + while remaining > 0: + this_limit = min(page_size, remaining) + pages.append((current_offset, this_limit)) + current_offset += this_limit + remaining -= this_limit + + failed_offsets: list[int] = [] + with ThreadPoolExecutor(max_workers=self.client.num_workers) as executor: + future_to_offset = { + executor.submit( + self._fetch_page, data, params, page_offset, page_limit + ): page_offset + for page_offset, page_limit in pages + } + for future in as_completed(future_to_offset): + page_offset = future_to_offset[future] + try: + all_data.append(future.result()) + except Exception: # surfaced as a warning below + failed_offsets.append(page_offset) + + if failed_offsets: + failed_offsets.sort() + actually_retrieved = sum( + (page.get("metadata") or {}).get("results", {}).get("returned_count", 0) + for page in all_data + ) + warn_incomplete_pages(failed_offsets, target, actually_retrieved) + all_data[0].setdefault("_parcllabs", {})["incomplete_pages"] = failed_offsets + + if target < total_available: + warn_truncation(target, total_available) return all_data @@ -241,8 +306,9 @@ def _get_metadata(self, results: list[Mapping[str, Any]]) -> dict[str, Any]: if not results: return {} - # Start with a copy of the first result's metadata - metadata = results[0].get("metadata", {}).copy() + # Deep copy: a shallow .copy() leaves metadata["results"] aliased to the raw + # first page, so assigning returned_count below would mutate the response. + metadata = copy.deepcopy(results[0].get("metadata", {})) # Calculate total returned_count total_returned = sum( @@ -252,6 +318,11 @@ def _get_metadata(self, results: list[Mapping[str, Any]]) -> dict[str, Any]: if "results" in metadata: metadata["results"]["returned_count"] = total_returned + # Surface any pages that could not be fetched (see _fetch_post). + incomplete = (results[0].get("_parcllabs") or {}).get("incomplete_pages") + if incomplete: + metadata["incomplete_pages"] = incomplete + return metadata def _build_search_criteria( @@ -430,20 +501,30 @@ def _build_owner_filters(self, params: PropertyV2RetrieveParams) -> dict[str, An return owner_filters - def _set_limit_pagination(self, limit: int | None) -> tuple[int, bool]: - """Validate and set limit and auto pagination.""" + def _set_limit_pagination(self, limit: int | None) -> tuple[int, int | None]: + """Resolve the caller's ``limit`` into a page size and a total cap. + + ``limit`` is the maximum number of *properties* to return in total, not a + page size. Pagination is an internal detail of satisfying it. + + Args: + limit: Maximum properties to return. ``None`` (or ``0``) means no cap. + + Returns: + ``(page_size, max_results)`` where ``max_results`` is ``None`` when + unbounded. + """ max_limit = RequestLimits.PROPERTY_V2_MAX.value - # If no limit is provided, use maximum limit and auto paginate + # `0` is treated as "no cap" defensively only. Callers cannot reach this via + # retrieve(): PropertyV2RetrieveParams enforces ge=1, so 0 and negatives are + # already rejected at validation time. if limit == 0 or limit is None: - auto_paginate = True - print(f"""No limit provided. Using max limit of {max_limit}. - Auto pagination is {auto_paginate}""") - return max_limit, auto_paginate + return max_limit, None - auto_paginate = False - print(f"Limit is set at {limit}. Auto pagiation is {auto_paginate}") - return limit, auto_paginate + # A limit above the API's per-request ceiling is satisfied by paginating, + # rather than by letting the server reject the request outright. + return min(limit, max_limit), limit def _build_param_categories( self, params: PropertyV2RetrieveParams @@ -539,7 +620,16 @@ def retrieve( current_entity_owner_name: Current entity owner name to filter by. include_events: Whether to include events in the response. include_full_event_history: Whether to include full event history in the response. - limit: Number of results to return. + limit: Maximum number of *properties* to return in total. Pagination is + handled internally to satisfy it, so values above the API's + per-request ceiling (50,000) are fetched across several pages rather + than rejected. Omit (or pass None) to retrieve every matching + property. Two things to note: credits are charged per property + returned, not per event; and because the returned DataFrame is + event-level, ``len(df)`` is NOT bounded by ``limit`` -- one property + may contribute many rows. If ``limit`` withholds matching data, a + ParclLabsTruncationWarning is emitted and + ``metadata["results"]`` reports both counts. params: Additional parameters to pass to the request. Returns: A tuple containing (pandas DataFrame, metadata dictionary). @@ -604,19 +694,22 @@ def retrieve( # Update data with categories data.update(param_categories.model_dump(exclude_none=True)) - # Set limit + # Set limit. `auto_paginate` is deliberately NOT placed in request_params -- + # it is an internal concern and was previously leaking into the query string. request_params = input_params.params.copy() - request_params["auto_paginate"] = False # auto_paginate is False by default # Make request with params if data.get(PARCL_PROPERTY_IDS): + # Querying by explicit property IDs: the ID list bounds the result, so a + # single page of PARCL_PROPERTY_IDS_LIMIT can always hold it, and >that + # many IDs are chunked in _fetch_post_parcl_property_ids. The caller's + # `limit` is not honoured on this path (breaking change -> DAT-122). request_params["limit"] = PARCL_PROPERTY_IDS_LIMIT results = self._fetch_post_parcl_property_ids(params=request_params, data=data) else: - request_params["limit"], request_params["auto_paginate"] = self._set_limit_pagination( - input_params.limit - ) - results = self._fetch_post(params=request_params, data=data) + page_size, max_results = self._set_limit_pagination(input_params.limit) + request_params["limit"] = page_size + results = self._fetch_post(params=request_params, data=data, max_results=max_results) # Get metadata from results metadata = self._get_metadata(results) @@ -624,4 +717,28 @@ def retrieve( # Process results final_df = self._as_pd_dataframe(results) + self._check_pagination_integrity(final_df, metadata) + return final_df, metadata + + @staticmethod + def _check_pagination_integrity(final_df: pd.DataFrame, metadata: dict[str, Any]) -> None: + """Warn if assembled pages did not yield the expected property count. + + Offset pagination is only safe while the server applies a stable sort. This + is a cheap guard so an upstream ordering change surfaces here rather than as + silently duplicated or missing rows in a customer's dataset. + """ + if final_df.empty or "parcl_property_id" not in final_df.columns: + return + # Pages that failed outright are already reported; don't double-warn. + if metadata.get("incomplete_pages"): + return + + expected = (metadata.get("results") or {}).get("returned_count") + if not expected: + return + + unique_properties = final_df["parcl_property_id"].nunique() + if unique_properties != expected: + warn_integrity_mismatch(unique_properties, expected) diff --git a/parcllabs/warnings.py b/parcllabs/warnings.py new file mode 100644 index 0000000..0e18569 --- /dev/null +++ b/parcllabs/warnings.py @@ -0,0 +1,96 @@ +"""Runtime warnings emitted by the ParclLabs SDK. + +These are warnings rather than prints so that callers can filter, capture, or +escalate them: + + import warnings + from parcllabs.warnings import ParclLabsTruncationWarning + + # silence intentional truncation + warnings.filterwarnings("ignore", category=ParclLabsTruncationWarning) + + # or turn incomplete results into a hard failure + warnings.filterwarnings("error", category=ParclLabsIncompleteResultWarning) +""" + +import warnings + + +class ParclLabsWarning(UserWarning): + """Base class for all ParclLabs SDK runtime warnings.""" + + +class ParclLabsTruncationWarning(ParclLabsWarning): + """More data matched the query than was returned. + + Emitted when an explicit ``limit`` capped the result below the number of + matching properties. The returned data is correct, just partial -- this is + what the caller asked for. Emitted once per session to stay usable inside + per-market loops. + """ + + +class ParclLabsIncompleteResultWarning(ParclLabsWarning): + """One or more pages could not be fetched, so the result is short. + + Unlike truncation, this is a failure rather than a request: the caller asked + for data the SDK was unable to retrieve. Emitted on every affected call, and + the failed page offsets are recorded in ``metadata["incomplete_pages"]``. + """ + + +# Truncation is a requested outcome, so it is announced once per process rather +# than on every call. A module-level flag is used deliberately: mutating the +# global `warnings` filters from library code would clobber caller configuration. +_truncation_warned = False + + +def warn_truncation(returned_count: int, total_available: int, stacklevel: int = 4) -> None: + """Warn once per session that an explicit ``limit`` withheld matching data.""" + global _truncation_warned # noqa: PLW0603 + if _truncation_warned: + return + _truncation_warned = True + warnings.warn( + f"Returned {returned_count:,} of {total_available:,} matching properties because " + f"`limit` capped the result. Raise `limit` to retrieve more, or omit it entirely " + f"to retrieve all {total_available:,}. Credits are charged per property returned. " + f"Compare metadata['results']['returned_count'] against " + f"metadata['results']['total_available'] to detect this programmatically. " + f"(This warning is shown once per session.)", + ParclLabsTruncationWarning, + stacklevel=stacklevel, + ) + + +def warn_incomplete_pages( + failed_offsets: list[int], expected: int, retrieved: int, stacklevel: int = 4 +) -> None: + """Warn that pagination could not fetch every page. Fires on every occurrence.""" + warnings.warn( + f"Incomplete result: {len(failed_offsets)} page(s) failed after retries, so " + f"{retrieved:,} of an expected {expected:,} properties were retrieved. Failed " + f"offsets are listed in metadata['incomplete_pages']. Re-run those offsets or " + f"retry the query before treating this data as complete. Failed offsets: " + f"{failed_offsets}", + ParclLabsIncompleteResultWarning, + stacklevel=stacklevel, + ) + + +def warn_integrity_mismatch(unique_properties: int, expected: int, stacklevel: int = 4) -> None: + """Warn that assembled pages did not yield the expected number of properties.""" + warnings.warn( + f"Pagination integrity check failed: assembled {unique_properties:,} unique " + f"properties but expected {expected:,}. This can indicate overlapping or skipped " + f"pages (offset pagination relies on a stable server-side sort). The data is " + f"returned as-is; verify before use and report this to team@parcllabs.com.", + ParclLabsIncompleteResultWarning, + stacklevel=stacklevel, + ) + + +def _reset_truncation_warning() -> None: + """Reset the once-per-session truncation flag. Intended for tests.""" + global _truncation_warned # noqa: PLW0603 + _truncation_warned = False diff --git a/tests/test_property_v2.py b/tests/test_property_v2.py index b06e7d4..d9088de 100644 --- a/tests/test_property_v2.py +++ b/tests/test_property_v2.py @@ -1,7 +1,11 @@ +import warnings from unittest.mock import MagicMock, Mock, patch +import pandas as pd import pytest +from requests.exceptions import RequestException +from parcllabs import warnings as parcllabs_warnings from parcllabs.common import PARCL_PROPERTY_IDS from parcllabs.enums import RequestLimits from parcllabs.schemas.schemas import GeoCoordinates, PropertyV2RetrieveParams @@ -209,12 +213,70 @@ def test_schema_with_none_values() -> None: assert params.params == {} -def test_validate_limit(property_v2_service: PropertyV2Service) -> None: - assert property_v2_service._set_limit_pagination(limit=None) == ( +def test_set_limit_pagination_resolves_page_size_and_cap( + property_v2_service: PropertyV2Service, +) -> None: + max_limit = RequestLimits.PROPERTY_V2_MAX.value + + # No limit -> max page size, no cap (retrieve everything). + assert property_v2_service._set_limit_pagination(limit=None) == (max_limit, None) + + # An explicit limit is a TOTAL CAP; page size matches it while under the ceiling. + assert property_v2_service._set_limit_pagination(limit=100) == (100, 100) + + # A limit above the API ceiling is satisfied by paginating, not by erroring. + assert property_v2_service._set_limit_pagination(limit=120_000) == (max_limit, 120_000) + + +def test_limit_zero_treated_as_unbounded_defensively( + property_v2_service: PropertyV2Service, +) -> None: + """0 falls back to 'no cap' in the helper, but callers cannot reach it.""" + assert property_v2_service._set_limit_pagination(limit=0) == ( RequestLimits.PROPERTY_V2_MAX.value, - True, + None, ) - assert property_v2_service._set_limit_pagination(limit=100) == (100, False) + + +@pytest.mark.parametrize("bad_limit", [0, -1, -5000]) +def test_non_positive_limit_rejected_at_validation(bad_limit: int) -> None: + """The schema rejects 0 and negatives, so a computed zero cannot trigger an + unbounded pull.""" + with pytest.raises(ValueError, match="greater than or equal to 1"): + PropertyV2RetrieveParams(limit=bad_limit) + + +def test_limit_above_api_ceiling_is_accepted() -> None: + """Values above the per-request ceiling must validate; pagination satisfies them.""" + assert PropertyV2RetrieveParams(limit=120_000).limit == 120_000 + + +def _page( + parcl_property_id: int, + *, + total_available: int, + returned_count: int = 1, + limit: int = 1, + offset: int = 0, + has_more: bool = False, +) -> Mock: + """Build a mock page response.""" + response = Mock() + response.json.return_value = { + "data": [{"parcl_property_id": parcl_property_id}], + "metadata": { + "results": {"total_available": total_available, "returned_count": returned_count} + }, + "pagination": {"limit": limit, "offset": offset, "has_more": has_more}, + "account_info": {"credits_used": returned_count, "credits_remaining": 999}, + } + return response + + +@pytest.fixture(autouse=True) +def _reset_truncation_flag() -> None: + """Truncation warns once per session; reset between tests.""" + parcllabs_warnings._reset_truncation_warning() @patch.object(PropertyV2Service, "_post") @@ -222,7 +284,7 @@ def test_fetch_post_single_page( mock_post: Mock, property_v2_service: PropertyV2Service, mock_response: Mock ) -> None: mock_post.return_value = mock_response - result = property_v2_service._fetch_post(params={"auto_paginate": False}, data={}) + result = property_v2_service._fetch_post(params={"limit": 100}, data={}) assert len(result) == 1 assert result[0] == mock_response.json() @@ -231,32 +293,162 @@ def test_fetch_post_single_page( @patch.object(PropertyV2Service, "_post") def test_fetch_post_pagination(mock_post: Mock, property_v2_service: PropertyV2Service) -> None: - # First response with pagination - first_response = Mock() - first_response.json.return_value = { - "data": [{"parcl_id": 123}], - "metadata": {"results": {"total_available": 2, "returned_count": 1}}, - "pagination": {"limit": 1, "offset": 0, "has_more": True}, - "account_info": {"credits_used": 1, "credits_remaining": 999}, - } + """Unbounded fetch must actually walk every page and merge the results.""" + mock_post.side_effect = [ + _page(123, total_available=2, limit=1, offset=0, has_more=True), + _page(456, total_available=2, limit=1, offset=1, has_more=False), + ] - # Second response for pagination - second_response = Mock() - second_response.json.return_value = { - "data": [{"parcl_id": 456}], - "metadata": {"results": {"total_available": 2, "returned_count": 1}}, - "pagination": {"limit": 1, "offset": 1, "has_more": False}, - "account_info": {"credits_used": 1, "credits_remaining": 998}, - } + result = property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=None) - # Set up the mock to return different responses - mock_post.side_effect = [first_response, second_response] + assert mock_post.call_count == 2 + assert [page["data"][0]["parcl_property_id"] for page in result] == [123, 456] - result = property_v2_service._fetch_post(params={"limit": 1, "auto_paginate": False}, data={}) - assert len(result) == 1 - assert result[0]["data"][0]["parcl_id"] == 123 +@patch.object(PropertyV2Service, "_post") +def test_fetch_post_stops_at_max_results( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """An explicit cap must not keep paginating toward total_available.""" + mock_post.return_value = _page(123, total_available=100, limit=1, offset=0, has_more=True) + + result = property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=1) + assert mock_post.call_count == 1 + assert len(result) == 1 + + +@patch.object(PropertyV2Service, "_post") +def test_fetch_post_trims_final_page_to_cap( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """A cap that is not a multiple of the page size must not overshoot.""" + mock_post.side_effect = [ + _page(1, total_available=100, returned_count=10, limit=10, offset=0, has_more=True), + _page(2, total_available=100, returned_count=5, limit=5, offset=10, has_more=True), + ] + + property_v2_service._fetch_post(params={"limit": 10}, data={}, max_results=15) + + assert mock_post.call_count == 2 + # Second call asks for only the outstanding 5, at the correct offset. + second_params = mock_post.call_args_list[1][1]["params"] + assert second_params["limit"] == 5 + assert second_params["offset"] == 10 + + +@patch.object(PropertyV2Service, "_post") +def test_truncation_warning_fires_once_per_session( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + mock_post.return_value = _page(1, total_available=108_288, limit=1, offset=0, has_more=True) + + with pytest.warns(parcllabs_warnings.ParclLabsTruncationWarning, match="108,288"): + property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=1) + + # Second identical call is silent -- once per session. + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=1) + assert not [w for w in caught if w.category is parcllabs_warnings.ParclLabsTruncationWarning] + + +@patch.object(PropertyV2Service, "_post") +def test_no_truncation_warning_when_complete( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + mock_post.return_value = _page(1, total_available=1, limit=1, offset=0, has_more=False) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=1) + + assert not [w for w in caught if w.category is parcllabs_warnings.ParclLabsTruncationWarning] + + +@patch.object(PropertyV2Service, "_post") +def test_page_retry_recovers_from_transient_failure( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + mock_post.side_effect = [ + _page(1, total_available=2, limit=1, offset=0, has_more=True), + RequestException("boom"), + _page(2, total_available=2, limit=1, offset=1, has_more=False), + ] + + with patch("parcllabs.services.properties.property_v2.time.sleep"): + result = property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=None) + + assert mock_post.call_count == 3 + assert len(result) == 2 + + +@patch.object(PropertyV2Service, "_post") +def test_incomplete_pages_warn_every_call( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """Exhausted retries must warn on EVERY call and record the failed offsets.""" + first = _page(1, total_available=2, limit=1, offset=0, has_more=True) + + def responses() -> list[object]: + return [first, *[RequestException("boom")] * 3] + + for _ in range(2): + mock_post.reset_mock() + mock_post.side_effect = responses() + with ( + patch("parcllabs.services.properties.property_v2.time.sleep"), + pytest.warns(parcllabs_warnings.ParclLabsIncompleteResultWarning), + ): + result = property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=None) + assert result[0]["_parcllabs"]["incomplete_pages"] == [1] + + +def test_incomplete_pages_surfaced_in_metadata(property_v2_service: PropertyV2Service) -> None: + results = [ + { + "metadata": {"results": {"returned_count": 1, "total_available": 2}}, + "_parcllabs": {"incomplete_pages": [1]}, + } + ] + assert property_v2_service._get_metadata(results)["incomplete_pages"] == [1] + + +def test_get_metadata_does_not_mutate_response(property_v2_service: PropertyV2Service) -> None: + """A shallow copy would rewrite returned_count on the caller's raw page.""" + results = [ + {"metadata": {"results": {"returned_count": 2, "total_available": 5}}}, + {"metadata": {"results": {"returned_count": 3, "total_available": 5}}}, + ] + + metadata = property_v2_service._get_metadata(results) + + assert metadata["results"]["returned_count"] == 5 + assert results[0]["metadata"]["results"]["returned_count"] == 2 + + +def test_integrity_check_warns_on_property_count_mismatch( + property_v2_service: PropertyV2Service, +) -> None: + # Two rows but only one distinct property, against a reported count of 2. + final_df = pd.DataFrame({"parcl_property_id": [1, 1]}) + metadata = {"results": {"returned_count": 2, "total_available": 2}} + + with pytest.warns(parcllabs_warnings.ParclLabsIncompleteResultWarning, match="integrity"): + property_v2_service._check_pagination_integrity(final_df, metadata) + + +def test_integrity_check_silent_when_counts_agree( + property_v2_service: PropertyV2Service, +) -> None: + final_df = pd.DataFrame({"parcl_property_id": [1, 1, 2]}) + metadata = {"results": {"returned_count": 2, "total_available": 2}} + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + property_v2_service._check_pagination_integrity(final_df, metadata) + + assert not caught def test_as_pd_dataframe(property_v2_service: PropertyV2Service, mock_response: Mock) -> None: @@ -310,9 +502,12 @@ def test_retrieve( # check that the metadata is returned assert metadata == mock_response.json()["metadata"] - # check that the correct data was passed to _fetch_post + # `limit` is the page size here; `auto_paginate` must NOT leak into the query + # string (it previously did, and was asserted as correct). call_args = mock_fetch_post.call_args[1] - assert call_args["params"] == {"limit": 10, "auto_paginate": False} + assert call_args["params"] == {"limit": 10} + assert "auto_paginate" not in call_args["params"] + assert call_args["max_results"] == 10 data = call_args["data"] assert data["parcl_ids"] == [123] From 60fc7a090bbeeaacbec848322f780dc9e5e388d7 Mon Sep 17 00:00:00 2001 From: zshamroukh Date: Tue, 4 Aug 2026 16:24:50 -0400 Subject: [PATCH 2/5] docs: document limit-as-total-cap, credit basis, and the new warnings (DAT-121) The README's limit guidance predated the pagination fix: it framed limit as a sampling tool without noting that values above the API's 50,000 per-request maximum are now paginated, that credits bill per property rather than per event, or that len(df) is not bounded by limit because the frame is event-level. Also documents the total_available vs returned_count assertion so callers have a programmatic truncation check, and the metadata["incomplete_pages"] contract. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index 9d5cb49..4677222 100644 --- a/README.md +++ b/README.md @@ -480,6 +480,24 @@ Gets a list of unique properties and their associated metadata and events based **NOTE:** Use the `limit` parameter to specify the number of matched properties to return. If `limit` is not provided, all matched properties will be returned. Conceptually, you should set the `limit` to retrieve a sample of properties, and then if you want to retrieve all properties, make the same request again without the `limit` parameter. +`limit` is a cap on the total number of **properties** returned, and pagination is handled for you — values larger than the API's 50,000 per-request maximum are fetched across multiple pages rather than rejected. Two things to keep in mind: + +- **Credits are charged per property returned, not per event.** +- The returned DataFrame is event-level, so `len(df)` is *not* bounded by `limit` — a single property can contribute many rows. + +If `limit` caps the result below the number of matching properties, a `ParclLabsTruncationWarning` is emitted (once per session) and both counts are available in the returned metadata. To check for this programmatically: + +```python +results, metadata = client.property_v2.search.retrieve(parcl_ids=[2900187], limit=1000) + +counts = metadata["results"] +assert counts["returned_count"] == counts["total_available"], ( + f"Truncated: got {counts['returned_count']:,} of {counts['total_available']:,} properties" +) +``` + +If any page fails after retries, the data is still returned but a `ParclLabsIncompleteResultWarning` is raised and the failed offsets are listed in `metadata["incomplete_pages"]`. Treat a non-empty `incomplete_pages` as an incomplete dataset. Both warning types live in `parcllabs.warnings` and can be silenced or escalated with standard `warnings` filters. + Example request, note that only one of `parcl_ids`, `parcl_property_ids`, or `geo_coordinates` can be provided per request: From 3a8cfdeb1653d012e61159141edff86bf6449a36 Mon Sep 17 00:00:00 2001 From: zshamroukh Date: Tue, 4 Aug 2026 16:35:27 -0400 Subject: [PATCH 3/5] fix(docs): make the README truncation example executable CI's test-readme step extracts every ```python block from the README and runs it against the live API. The example I added set limit=1000 and then asserted returned_count == total_available, which is false by construction -- it failed CI on exactly the truncation it was demonstrating. Rewritten as a reporting check rather than an assertion, which is also better guidance: a reader copying it does not get a crash. Prose notes that you can make it fatal if your pipeline wants that. Also scoped the warnings-filter example inside warnings.catch_warnings(). As written it set filterwarnings("error", ParclLabsIncompleteResultWarning) globally, and since all README blocks are concatenated into one script that would have made any later example raise on a partial page or integrity mismatch. Dropped the example limit from 1000 to 5 -- it still truncates against 4M matching properties, so it demonstrates the same thing for 5 credits per CI run instead of 1,000. Verified with `make test-readme` locally: exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 4677222..897a587 100644 --- a/README.md +++ b/README.md @@ -485,18 +485,40 @@ Gets a list of unique properties and their associated metadata and events based - **Credits are charged per property returned, not per event.** - The returned DataFrame is event-level, so `len(df)` is *not* bounded by `limit` — a single property can contribute many rows. -If `limit` caps the result below the number of matching properties, a `ParclLabsTruncationWarning` is emitted (once per session) and both counts are available in the returned metadata. To check for this programmatically: +If `limit` caps the result below the number of matching properties, a `ParclLabsTruncationWarning` is emitted (once per session) and both counts are available in the returned metadata. If any page fails after retries, the data is still returned but a `ParclLabsIncompleteResultWarning` is raised and the failed offsets are listed in `metadata["incomplete_pages"]`. + +Both conditions are worth checking programmatically, especially in a loop over many markets where a warning is easy to miss: ```python -results, metadata = client.property_v2.search.retrieve(parcl_ids=[2900187], limit=1000) +results, metadata = client.property_v2.search.retrieve(parcl_ids=[2900187], limit=5) counts = metadata["results"] -assert counts["returned_count"] == counts["total_available"], ( - f"Truncated: got {counts['returned_count']:,} of {counts['total_available']:,} properties" -) +if counts["returned_count"] < counts["total_available"]: + print( + f"Truncated: got {counts['returned_count']:,} of " + f"{counts['total_available']:,} properties. Raise `limit`, or omit it entirely." + ) + +if metadata.get("incomplete_pages"): + print(f"Incomplete: pages failed at offsets {metadata['incomplete_pages']}") ``` -If any page fails after retries, the data is still returned but a `ParclLabsIncompleteResultWarning` is raised and the failed offsets are listed in `metadata["incomplete_pages"]`. Treat a non-empty `incomplete_pages` as an incomplete dataset. Both warning types live in `parcllabs.warnings` and can be silenced or escalated with standard `warnings` filters. +If a short result should be fatal for your pipeline, make those checks `assert`s or raise your own exception — treat a non-empty `incomplete_pages` as an incomplete dataset either way. Both warning types live in `parcllabs.warnings` and can be silenced or escalated with standard `warnings` filters: + +```python +import warnings + +from parcllabs.warnings import ParclLabsIncompleteResultWarning, ParclLabsTruncationWarning + +with warnings.catch_warnings(): + # Intentionally sampling? Silence the truncation notice. + warnings.filterwarnings("ignore", category=ParclLabsTruncationWarning) + + # Never accept a partial page silently -- make it raise instead. + warnings.filterwarnings("error", category=ParclLabsIncompleteResultWarning) + + results, metadata = client.property_v2.search.retrieve(parcl_ids=[2900187], limit=5) +``` Example request, note that only one of `parcl_ids`, `parcl_property_ids`, or `geo_coordinates` can be provided per request: From e01624c1ca747a1f1230e7e3c94dc19dcffa2a35 Mon Sep 17 00:00:00 2001 From: zshamroukh Date: Tue, 4 Aug 2026 16:42:00 -0400 Subject: [PATCH 4/5] fix: do not claim the cap was met when pages failed (Bugbot) When pagination lost pages, _fetch_post emitted the incomplete-result warning with the real retrieved count and then still called warn_truncation with `target`. Three problems: - the truncation message claimed the full cap was returned when fewer properties had actually been fetched - it directly contradicted the incomplete warning issued a line earlier - truncation fires once per session, so it burned that budget on the misleading message and would have suppressed a legitimate truncation notice later in the same run Failed pages now return early and warn only about incompleteness, which is the stronger and non-contradictory signal. `total_available` is passed into that warning so capping information is not lost by skipping the truncation notice. Also made the truncation message report what was actually returned rather than what was requested, in both the early-return and paginated paths, and extracted the page-sum into _total_returned. Two regression tests: failed pages must not emit a truncation warning and must leave the once-per-session flag intact; the truncation message must state the returned count, not the requested one. Co-Authored-By: Claude Opus 5 (1M context) --- parcllabs/services/properties/property_v2.py | 29 ++++++++---- parcllabs/warnings.py | 19 ++++++-- tests/test_property_v2.py | 49 ++++++++++++++++++++ 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/parcllabs/services/properties/property_v2.py b/parcllabs/services/properties/property_v2.py index 2e6584c..96dac31 100644 --- a/parcllabs/services/properties/property_v2.py +++ b/parcllabs/services/properties/property_v2.py @@ -93,8 +93,9 @@ def _fetch_post( target = total_available if max_results is None else min(max_results, total_available) if retrieved >= target or not pagination.get("has_more"): - if target < total_available: - warn_truncation(target, total_available) + # Report what was actually returned, not what was requested. + if retrieved < total_available: + warn_truncation(retrieved, total_available) return all_data page_size = pagination.get("limit") or params.get("limit") or retrieved @@ -126,20 +127,32 @@ def _fetch_post( except Exception: # surfaced as a warning below failed_offsets.append(page_offset) + actually_retrieved = self._total_returned(all_data) + if failed_offsets: + # Report the shortfall and stop. Deliberately no truncation warning here: + # we did NOT return `target`, so claiming we did would contradict this + # warning, and the truncation notice is once-per-session -- burning it on a + # misleading message would suppress a legitimate one later in the run. + # `total_available` is included so capping information is not lost. failed_offsets.sort() - actually_retrieved = sum( - (page.get("metadata") or {}).get("results", {}).get("returned_count", 0) - for page in all_data - ) - warn_incomplete_pages(failed_offsets, target, actually_retrieved) + warn_incomplete_pages(failed_offsets, target, actually_retrieved, total_available) all_data[0].setdefault("_parcllabs", {})["incomplete_pages"] = failed_offsets + return all_data if target < total_available: - warn_truncation(target, total_available) + warn_truncation(actually_retrieved, total_available) return all_data + @staticmethod + def _total_returned(pages: list[dict]) -> int: + """Sum the properties actually returned across assembled pages.""" + return sum( + (page.get("metadata") or {}).get("results", {}).get("returned_count", 0) + for page in pages + ) + def _fetch_post_parcl_property_ids( self, params: dict[str, Any], diff --git a/parcllabs/warnings.py b/parcllabs/warnings.py index 0e18569..8f374cf 100644 --- a/parcllabs/warnings.py +++ b/parcllabs/warnings.py @@ -64,15 +64,26 @@ def warn_truncation(returned_count: int, total_available: int, stacklevel: int = def warn_incomplete_pages( - failed_offsets: list[int], expected: int, retrieved: int, stacklevel: int = 4 + failed_offsets: list[int], + expected: int, + retrieved: int, + total_available: int | None = None, + stacklevel: int = 4, ) -> None: - """Warn that pagination could not fetch every page. Fires on every occurrence.""" + """Warn that pagination could not fetch every page. Fires on every occurrence. + + This supersedes the truncation warning when both would apply: the result is short + because pages failed, not because the caller asked for less. + """ + matched = "" + if total_available is not None and total_available > expected: + matched = f" ({total_available:,} properties matched the query in total.)" warnings.warn( f"Incomplete result: {len(failed_offsets)} page(s) failed after retries, so " f"{retrieved:,} of an expected {expected:,} properties were retrieved. Failed " f"offsets are listed in metadata['incomplete_pages']. Re-run those offsets or " - f"retry the query before treating this data as complete. Failed offsets: " - f"{failed_offsets}", + f"retry the query before treating this data as complete.{matched} Failed " + f"offsets: {failed_offsets}", ParclLabsIncompleteResultWarning, stacklevel=stacklevel, ) diff --git a/tests/test_property_v2.py b/tests/test_property_v2.py index d9088de..fbcfab5 100644 --- a/tests/test_property_v2.py +++ b/tests/test_property_v2.py @@ -404,6 +404,55 @@ def responses() -> list[object]: assert result[0]["_parcllabs"]["incomplete_pages"] == [1] +@patch.object(PropertyV2Service, "_post") +def test_failed_pages_suppress_contradictory_truncation_warning( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """A capped request whose pages fail must not also claim the cap was met. + + Regression: _fetch_post emitted the incomplete warning with the real retrieved + count and *then* warned truncation with `target`, claiming the full cap was + returned. The two messages contradicted each other, and the truncation notice -- + which fires only once per session -- was burned on the misleading one. + """ + first = _page(1, total_available=108_295, limit=1, offset=0, has_more=True) + mock_post.side_effect = [first, *[RequestException("boom")] * 3] + + with ( + patch("parcllabs.services.properties.property_v2.time.sleep"), + warnings.catch_warnings(record=True) as caught, + ): + warnings.simplefilter("always") + property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=2) + + incomplete = [ + w for w in caught if w.category is parcllabs_warnings.ParclLabsIncompleteResultWarning + ] + truncation = [w for w in caught if w.category is parcllabs_warnings.ParclLabsTruncationWarning] + + assert len(incomplete) == 1 + assert "1 of an expected 2" in str(incomplete[0].message) + # Capping information is preserved in the incomplete message instead. + assert "108,295" in str(incomplete[0].message) + assert not truncation, "truncation warning must not contradict the incomplete warning" + + # The once-per-session budget must be intact for a later, legitimate truncation. + assert parcllabs_warnings._truncation_warned is False + + +@patch.object(PropertyV2Service, "_post") +def test_truncation_reports_actual_not_requested_count( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """The truncation message must state what was returned, not what was asked for.""" + mock_post.return_value = _page(1, total_available=500, limit=1, offset=0, has_more=True) + + with pytest.warns(parcllabs_warnings.ParclLabsTruncationWarning) as caught: + property_v2_service._fetch_post(params={"limit": 1}, data={}, max_results=1) + + assert "Returned 1 of 500" in str(caught[0].message) + + def test_incomplete_pages_surfaced_in_metadata(property_v2_service: PropertyV2Service) -> None: results = [ { From e9180a06a6980a8807167112ce1eaa335cd69ddc Mon Sep 17 00:00:00 2001 From: zshamroukh Date: Tue, 4 Aug 2026 16:51:12 -0400 Subject: [PATCH 5/5] fix: only blame `limit` for a shortfall when a cap was actually set (Bugbot) Regression I introduced in e01624c. Fixing the truncation message to report the returned count rather than the requested one, I also changed the gate from `target < total_available` to `retrieved < total_available` -- one edit too many. That made the early-return path warn on ANY shortfall, including an uncapped request whose response ended pagination early (has_more=False while more properties matched). It told the caller their result was short "because `limit` capped the result" when no limit had been passed: unactionable advice, and it consumed the once-per-session budget that a later legitimate truncation needs. The gate and the count answer different questions. Gate on `target` (did a cap withhold data?), report `retrieved` (what did we actually return?). The multi-page path already did this correctly, so the two paths were inconsistent. Verified against the three relevant states: no cap, server ends early -> silent (was: warned, blaming limit) cap below available -> warns, honest count no cap, consistent -> silent Deliberately not warning at all in the first case: it requires an inconsistent server response, total_available remains in metadata, and misreporting it as truncation is worse than staying quiet. Routing it to ParclLabsIncompleteResultWarning instead is a possible follow-up. Co-Authored-By: Claude Opus 5 (1M context) --- parcllabs/services/properties/property_v2.py | 8 ++++++-- tests/test_property_v2.py | 21 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/parcllabs/services/properties/property_v2.py b/parcllabs/services/properties/property_v2.py index 96dac31..7b4ada8 100644 --- a/parcllabs/services/properties/property_v2.py +++ b/parcllabs/services/properties/property_v2.py @@ -93,8 +93,12 @@ def _fetch_post( target = total_available if max_results is None else min(max_results, total_available) if retrieved >= target or not pagination.get("has_more"): - # Report what was actually returned, not what was requested. - if retrieved < total_available: + # Gate on whether a cap actually withheld data (`target`), but report the + # count actually returned (`retrieved`). Gating on `retrieved` instead would + # also fire when no cap was set and the server merely ended pagination early, + # blaming `limit` for a shortfall it did not cause -- and burning the + # once-per-session budget on advice the caller cannot act on. + if target < total_available: warn_truncation(retrieved, total_available) return all_data diff --git a/tests/test_property_v2.py b/tests/test_property_v2.py index fbcfab5..0ded210 100644 --- a/tests/test_property_v2.py +++ b/tests/test_property_v2.py @@ -440,6 +440,27 @@ def test_failed_pages_suppress_contradictory_truncation_warning( assert parcllabs_warnings._truncation_warned is False +@patch.object(PropertyV2Service, "_post") +def test_no_truncation_warning_when_no_limit_was_set( + mock_post: Mock, property_v2_service: PropertyV2Service +) -> None: + """A shortfall with no cap in play must not be blamed on `limit`. + + Regression: the early-return path gated on `retrieved < total_available`, so an + uncapped request whose server response ended pagination early (has_more=False + while more properties matched) warned "because `limit` capped the result" -- advice + the caller cannot act on, and it consumed the once-per-session budget. + """ + mock_post.return_value = _page(1, total_available=100, returned_count=50, has_more=False) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + property_v2_service._fetch_post(params={"limit": 1000}, data={}, max_results=None) + + assert not [w for w in caught if w.category is parcllabs_warnings.ParclLabsTruncationWarning] + assert parcllabs_warnings._truncation_warned is False + + @patch.object(PropertyV2Service, "_post") def test_truncation_reports_actual_not_requested_count( mock_post: Mock, property_v2_service: PropertyV2Service