Repository navigation
feat(ingest): add --skip-existing to skip already-ingested files - #968
Merged
Merged
Conversation
Adds `ref datasets ingest --skip-existing`. Files whose path is already in the database are left out before parsing, so a periodic ingest only opens new files. This assumes files are immutable, so a full ingest is still needed to pick up changes. With `--chunk-size` the database is asked about each chunk of discovered paths, so memory stays bounded by the chunk size rather than the archive size. Adds an index on `dataset_file.path` to keep those lookups off a table scan.
📝 Walkthrough
Priority: ⬇️ Low Change: Feature Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description gives detailed and relevant technical context, but it does not use the required ## Description and ## Checklist sections or confirm the tests, documentation, and changelog checklis… |
Add the required ## Description and ## Checklist headings. Include checked confirmations for tests added, documentation added where applicable, and the changelog item added to changelog/. |
| Docstring Coverage | |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 15 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely summarises the main change: adding --skip-existingto skip files that were already ingested.Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description gives detailed and relevant technical context, but it does not use the required
## Descriptionand## Checklistsections or confirm the tests, documentation, and changelog checklist items.Resolution
Add the required
## Descriptionand## Checklistheadings. Include checked confirmations for tests added, documentation added where applicable, and the changelog item added tochangelog/.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Files left after skipping are batched back up to the chunk size, so a mostly skipped walk no longer starts a worker pool per tiny chunk. Moves the skip function factory next to the database lookup and batches lookups with itertools.batched.
…unks Adds iter_local_datasets to both reference adapters, so --chunk-size and --skip-existing keep memory bounded for every built-in adapter.
lewisjared
marked this pull request as ready for review
October 19, 2026 11:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5a2b3452-f31d-430a-a047-779403be8d83
📒 Files selected for processing: 16
changelog/968.feature.mdchangelog/968.improvement.mdpackages/climate-ref/src/climate_ref/cli/datasets.pypackages/climate-ref/src/climate_ref/datasets/__init__.pypackages/climate-ref/src/climate_ref/datasets/base.pypackages/climate-ref/src/climate_ref/datasets/catalog_builder.pypackages/climate-ref/src/climate_ref/datasets/cmip6.pypackages/climate-ref/src/climate_ref/datasets/cmip7.pypackages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.pypackages/climate-ref/src/climate_ref/datasets/obs4mips.pypackages/climate-ref/src/climate_ref/migrations/versions/2026-10-19T0000_f8a9b0c1d2e3_add_dataset_file_path_index.pypackages/climate-ref/src/climate_ref/models/dataset.pypackages/climate-ref/tests/unit/cli/test_datasets.pypackages/climate-ref/tests/unit/datasets/test_catalog_builder.pypackages/climate-ref/tests/unit/datasets/test_datasets.pypackages/climate-ref/tests/unit/datasets/test_esmvaltool_reference.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
… limit Only passes `skip` to adapters when --skip-existing is set, so adapters with the previous signature still ingest. Lowers the path lookup batch to 900, under the 999 parameter cap of SQLite before 3.32.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
aa4d94ee-3a8d-4ad4-8ad9-1e3dbff7e831
📒 Files selected for processing: 11
packages/climate-ref/src/climate_ref/cli/datasets.pypackages/climate-ref/src/climate_ref/datasets/__init__.pypackages/climate-ref/src/climate_ref/datasets/base.pypackages/climate-ref/src/climate_ref/datasets/catalog_builder.pypackages/climate-ref/src/climate_ref/datasets/cmip6.pypackages/climate-ref/src/climate_ref/datasets/cmip7.pypackages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.pypackages/climate-ref/src/climate_ref/datasets/obs4mips.pypackages/climate-ref/tests/unit/cli/test_datasets.pypackages/climate-ref/tests/unit/datasets/test_catalog_builder.pypackages/climate-ref/tests/unit/datasets/test_datasets.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…alid With --skip-existing, an archive of only invalid files returned an empty catalog and exited 0. build_catalog now raises the adapter's error when files were parsed but none were valid, and still returns an empty frame when skip removed every file.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Record parsed-but-invalid directories as failures in chunked mode. · datasets.py:374-387
packages/climate-ref/src/climate_ref/cli/datasets.py:374-387
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord parsed-but-invalid directories as failures in chunked mode.
When
--chunk-sizeand--skip-existingare enabled, the Obs4MIPs and ESMValTool iterators can parse the remaining files and yield no chunks when all files are invalid. The CLI then leavestotal_filesat zero, treats the directory as having no new files, and exits successfully without adding the directory tofailed_dirs.Pass the existing adapter-specific error to
iter_built_catalogs(). Raise it only when parsing occurred and no valid chunk was emitted. This preserves the successful all-skipped path.Suggested fix
def iter_built_catalogs( # noqa: PLR0913 paths: list[str], parsing_func: DatasetParsingFunction, include_patterns: list[str] | None = None, n_jobs: int = 1, chunk_size: int = 10_000, skip: SkipFunction | None = None, + error_if_none_valid: str | None = None, ) -> Iterator[pd.DataFrame]: ... if not any_emitted and (skip is None or any_parsed): + if error_if_none_valid is not None: + raise ValueError(error_if_none_valid) logger.warning(f"No valid files found in {paths} matching {include_patterns}")n_jobs=self.n_jobs, chunk_size=chunk_size, skip=skip, + error_if_none_valid="No obs4MIPs-compliant datasets found", ):n_jobs=self.n_jobs, chunk_size=chunk_size, skip=skip, + error_if_none_valid="No ESMValTool reference datasets found", ):
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
206de5f6-d97b-4993-898e-b9621e18c35b
📒 Files selected for processing: 5
packages/climate-ref/src/climate_ref/datasets/catalog_builder.pypackages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.pypackages/climate-ref/src/climate_ref/datasets/obs4mips.pypackages/climate-ref/tests/unit/datasets/test_catalog_builder.pypackages/climate-ref/tests/unit/datasets/test_obs4mips.py
🚧 Files skipped from review as they are similar to previous changes: 3
- packages/climate-ref/tests/unit/datasets/test_catalog_builder.py
- packages/climate-ref/src/climate_ref/datasets/obs4mips.py
- packages/climate-ref/src/climate_ref/datasets/catalog_builder.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
ref datasets ingest --skip-existing, which leaves out files whose path is already in the database before they are parsed. This makes a periodic ingest of a large archive only open new files. It assumes files are immutable, so a full ingest without the flag is still needed to pick up changed files.With
--chunk-size, the database is asked about each chunk of discovered paths, so memory is bounded by the chunk size rather than the size of the archive. Files left after skipping are batched back up to the chunk size, so a mostly skipped run doesn't start a worker pool for each tiny remainder. Without--chunk-sizethe whole file list is still held in memory, as before.Things worth a close look:
dataset_file.path, so the lookups don't scan the table. Dropping it is safe, but large deployments pay the index build on upgrade.find_local_datasetsanditer_local_datasetstake an optionalskipcallable. Third-party adapters that don't accept it keep working unless--skip-existingis used.iter_local_datasets, so--chunk-sizenow streams for every built-in adapter instead of falling back to the whole catalog.While sizing chunks I found that CMIP6
_apply_fixes(agroupby().apply) peaks at about 34 KB per dataset, around 350 MB for a 10k chunk. A vectorised version peaks at about 15 MB. That is left for a separate PR because it changes row order.Summary by CodeRabbit
--skip-existingto dataset ingestion, skipping files already in the database without parsing.--chunk-sizenow supports streaming built-in reference datasets, including obs4MIPs, obs4REF, PMP climatology and ESMValTool reference datasets.