Translate an empty ESGF search into DatasetResolutionError#811
Conversation
A dataset that has been de-indexed from ESGF made cat.search() raise intake_esgf's NoSearchResults, which escaped the per-test-case handling in ref test-cases fetch and crashed the whole prefetch. Raising DatasetResolutionError instead lets the fetch log and skip the affected test case.
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesESGF no-results handling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/climate-ref-core/src/climate_ref_core/esgf/base.py (1)
115-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winException translation is correctly implemented.
The
try/except NoSearchResultsblock properly catches the upstream exception and raisesDatasetResolutionErrorwith a descriptive facets-specific message. Usingfrom Noneto suppress the original traceback is appropriate here since the domain error message is self-explanatory.One observation: the pre-existing
ValueErrorat line 129 (if cat.df is None or cat.df.empty) represents a similar "no data" scenario but raises a different exception type. Ifcat.search()can succeed without raisingNoSearchResultsyet produce an empty DataFrame (e.g., afterremove_ensembles()strips all results), thatValueErrorwould still crash the prefetch. Consider whether this path should also be translated toDatasetResolutionErrorfor consistency with the PR's goal of graceful skipping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e3b067f-f74c-4eee-a050-b23e71942c6b
📒 Files selected for processing (3)
changelog/811.fix.mdpackages/climate-ref-core/src/climate_ref_core/esgf/base.pypackages/climate-ref-core/tests/unit/esgf/test_base.py
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Description
Translates intake-esgf's
NoSearchResultsintoDatasetResolutionErrorinIntakeESGFMixin.fetch_datasets.ref test-cases fetchalready logs and skips a test case that raisesDatasetResolutionError, so a single de-indexed dataset no longer crashes the whole prefetch.This is the failure behind the red CI Integration Tests run on the #810 merge commit. The obs4MIPs
C3S-GTO-ECV-9-0record (the toz reference for the ozone diagnostics) is currently missing from theESGF2-US-1.5-Catalogindex that CI queries, but it still exists on the CEDA index and in the sample-data registry. While the record is missing, the ozone test-case catalogs stop refreshing and the committed catalogs keep the downstream jobs working.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit