Add snapshot creation mechanism#39
Merged
Merged
Conversation
Closed
|
Warning Review limit reached
Next review available in: 35 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds reader discovery and per-invocation selection, snapshot metadata/statistics, local lineage-aware storage, ChangesSnapshot tracking and benchmark pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BenchmarkCLI
participant SyntheticCSV
participant BenchmarkRunner
participant SnapshotPipeline
participant ConsoleReporter
User->>BenchmarkCLI: run benchmark options
BenchmarkCLI->>SyntheticCSV: generate temporary dataset
BenchmarkCLI->>BenchmarkRunner: run generated CSV
BenchmarkRunner->>SnapshotPipeline: build metadata and compute statistics
BenchmarkRunner->>ConsoleReporter: report timings and call graph
ConsoleReporter-->>User: render benchmark results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dstrack/_benchmark/_profiling.py`:
- Around line 109-126: Update the root selection in the graph-building flow so
frames with at least one caller outside the scoped frame set are included as
roots, alongside frames not present in callees. Preserve recursive components by
tracking out-of-scope callers while iterating through callers, then pass the
combined root candidates to graph._by_cumulative.
In `@src/dstrack/_benchmark/_runner.py`:
- Around line 121-132: Reset the profiler at the start of each run in
BenchmarkRunner.run so reused BenchmarkRunner instances produce call graphs
containing only the current invocation. Recreate or clear self._profiler while
preserving the existing disabled behavior when profile is false.
In `@src/dstrack/_benchmark/_synthetic.py`:
- Around line 86-92: Update SyntheticCsvSpec in
src/dstrack/_benchmark/_synthetic.py to validate every num_* count as
non-negative and require null_rate to be finite and within [0, 1]. Update
CallGraph.trees in src/dstrack/_benchmark/_profiling.py to validate
profile_limit/max_children as non-negative before applying the child limit.
Update the Typer options in src/dstrack/_benchmark/_cli.py to expose the same
bounds and reject invalid values at CLI parsing.
In `@src/dstrack/readers/_registry.py`:
- Around line 143-154: Validate each normalized extension in the registration
flow before duplicate checks or any mutation of _BY_NAME and _BY_EXTENSION.
Require every extension in claimed to use the documented leading-dot format,
rejecting malformed values such as “csv” with a clear ValueError; preserve valid
extension registration behavior.
- Around line 79-105: The ReaderFactory validation in check_reader_class and
build_reader must verify that from_path is specifically a classmethod, not
merely an existing attribute, before invoking it. Use descriptor-level
inspection to enforce the classmethod contract, and validate that build_reader
returns a TabularReader, rejecting any other constructed value.
In `@src/dstrack/snapshot/_builder.py`:
- Around line 81-91: Update the snapshot-building flow around
_metadata_builder.build and _stats_computer.compute so source_hash and stats are
derived from the same immutable file state. Hash and compute statistics from one
read snapshot, or detect a file change between operations and abort or retry
instead of persisting mixed-state metadata.
In `@src/dstrack/snapshot/_stats.py`:
- Around line 131-132: Define and implement an explicit policy for non-finite
values in the stats accumulator: detect NaN and infinities in update and prevent
them from reaching histogram step or bucket calculations. Apply the same
handling in the related histogram-building paths at the noted ranges, and add
tests covering both NaN and infinite inputs according to the chosen policy.
- Around line 128-129: Update the snapshot statistics state around vals,
null_count, and the distinct-value tracking at the referenced logic to avoid
retaining unbounded input data in memory. Replace full numeric and row
collections with bounded sketches or spill-to-disk storage; if that is not
supported, enforce and document a clear maximum input size before processing.
In `@src/dstrack/store.py`:
- Around line 115-128: Serialize snapshot creation and recovery with a
cross-process lock covering parent selection, snapshot-file write, log append,
and HEAD update. Update recovery/path-matching logic to treat the snapshot
referenced by HEAD as the latest committed entry, ignoring any trailing or
orphaned log records after a crash; ensure concurrent writers cannot use the
same parent or leave log.jsonl ahead of HEAD.
- Around line 96-121: Validate dataset_id and snapshot_id as canonical UUIDs, or
at minimum reject path components containing separators or traversal segments,
before any filesystem access or path joins in the snapshot-writing flow. Apply
the checks before _check_dataset_exists, dataset directory construction, and
snapshot file construction, while preserving the existing lineage and snapshot
behavior for valid identifiers.
In `@tests/test_snapshot_metadata.py`:
- Around line 80-83: Update test_dataset_path_stored_as_string to define the
expected dataset path as a Path and compare meta.dataset_path with
str(expected_path), rather than asserting a hard-coded slash-separated string.
Preserve the test’s verification that the built metadata stores the path as a
string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9425a2ae-4d62-4c10-9aaa-52aa38a9b67f
📒 Files selected for processing (27)
docs/decisions/0003-local-snapshot-store-layout.mdpyproject.tomlsrc/dstrack/_benchmark/__init__.pysrc/dstrack/_benchmark/_cli.pysrc/dstrack/_benchmark/_environment.pysrc/dstrack/_benchmark/_profiling.pysrc/dstrack/_benchmark/_report.pysrc/dstrack/_benchmark/_runner.pysrc/dstrack/_benchmark/_synthetic.pysrc/dstrack/_cli.pysrc/dstrack/_track.pysrc/dstrack/errors.pysrc/dstrack/readers/__init__.pysrc/dstrack/readers/_csv.pysrc/dstrack/readers/_protocol.pysrc/dstrack/readers/_registry.pysrc/dstrack/readers/_resolve.pysrc/dstrack/snapshot/__init__.pysrc/dstrack/snapshot/_builder.pysrc/dstrack/snapshot/_metadata.pysrc/dstrack/snapshot/_stats.pysrc/dstrack/snapshot/_version.pysrc/dstrack/store.pytests/test_reader_registry.pytests/test_snapshot_metadata.pytests/test_snapshot_stats.pytests/test_store.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_benchmark_cli.py (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
field(default_factory=list)over a hand-written__init__on the dataclass.
_RecordingReporterdeclareskept_paths/reportedas dataclass fields but then overrides__init__to initialize them manually — this defeats the point of@dataclassgenerating__init__.♻️ Proposed fix
+from dataclasses import dataclass, field + `@dataclass` class _RecordingReporter: """Stands in for ConsoleReporter, recording what the CLI asked it to print.""" - kept_paths: list[Path] - reported: list[tuple[BenchmarkRun, int]] - - def __init__(self) -> None: - self.kept_paths = [] - self.reported = [] + kept_paths: list[Path] = field(default_factory=list) + reported: list[tuple[BenchmarkRun, int]] = field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_cli.py` around lines 22 - 31, Update the _RecordingReporter dataclass to use field(default_factory=list) for kept_paths and reported, and remove its hand-written __init__ so the dataclass-generated initializer handles both fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_benchmark_cli.py`:
- Around line 57-68: Update _StubRunner to declare instances as a class-level
ClassVar initialized to an empty list, and remove the per-instance
self.instances assignment from __init__. Keep type(self).instances.append(self)
so all constructed runners remain recorded through the class attribute without
relying on fixture setup.
---
Nitpick comments:
In `@tests/test_benchmark_cli.py`:
- Around line 22-31: Update the _RecordingReporter dataclass to use
field(default_factory=list) for kept_paths and reported, and remove its
hand-written __init__ so the dataclass-generated initializer handles both
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b24755f5-9dfc-4ec3-8523-22b4cdbf4761
📒 Files selected for processing (23)
codecov.yamldocs/API/snapshot.mdsrc/dstrack/_benchmark/__init__.pysrc/dstrack/_benchmark/_cli.pysrc/dstrack/_benchmark/_environment.pysrc/dstrack/_benchmark/_profiling.pysrc/dstrack/_benchmark/_report.pysrc/dstrack/_benchmark/_runner.pysrc/dstrack/_benchmark/_synthetic.pysrc/dstrack/_cli.pysrc/dstrack/_logging.pysrc/dstrack/paths.pysrc/dstrack/readers/__init__.pysrc/dstrack/readers/_csv.pysrc/dstrack/readers/_protocol.pysrc/dstrack/readers/_registry.pysrc/dstrack/readers/_resolve.pysrc/dstrack/snapshot/__init__.pysrc/dstrack/snapshot/_metadata.pysrc/dstrack/snapshot/_stats.pysrc/dstrack/snapshot/_version.pysrc/dstrack/store.pytests/test_benchmark_cli.py
🚧 Files skipped from review as they are similar to previous changes (18)
- src/dstrack/_benchmark/init.py
- src/dstrack/snapshot/_version.py
- src/dstrack/snapshot/init.py
- src/dstrack/readers/init.py
- src/dstrack/readers/_csv.py
- src/dstrack/_cli.py
- src/dstrack/snapshot/_metadata.py
- src/dstrack/_benchmark/_report.py
- src/dstrack/_benchmark/_synthetic.py
- src/dstrack/_benchmark/_runner.py
- src/dstrack/store.py
- src/dstrack/_benchmark/_profiling.py
- src/dstrack/_benchmark/_cli.py
- src/dstrack/readers/_protocol.py
- src/dstrack/readers/_resolve.py
- src/dstrack/_benchmark/_environment.py
- src/dstrack/readers/_registry.py
- src/dstrack/snapshot/_stats.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_track.py (1)
82-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading comment on directory-entry count.
The comment says "2 snapshots + HEAD file" but the dataset directory actually contains a
snapshots/subdirectory (holding the 2 JSON files),log.jsonl, andHEAD— three top-level entries, not two snapshot files plus HEAD. The assertion passes by coincidence (3 == 3); consider asserting the actual structure directly (e.g., snapshot count viaglob("snapshots/*.json"), and existence oflog.jsonl/HEAD) so a future change to the on-disk layout doesn't produce a confusing failure.✏️ Suggested clarification
- # Both snapshots live under the same dataset directory. - dataset_dirs = list((store / "datasets").iterdir()) - assert len(dataset_dirs) == 1 - # Snapshot dir should contain 2 snapshots + HEAD file - snapshot_dir = dataset_dirs[0] - assert len(list(snapshot_dir.iterdir())) == 3 + # Both snapshots live under the same dataset directory. + dataset_dirs = list((store / "datasets").iterdir()) + assert len(dataset_dirs) == 1 + dataset_dir = dataset_dirs[0] + assert len(list((dataset_dir / "snapshots").glob("*.json"))) == 2 + assert (dataset_dir / "log.jsonl").exists() + assert (dataset_dir / "HEAD").exists()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_track.py` around lines 82 - 86, Update the assertions following dataset_dirs in the track test to validate the actual on-disk structure: assert that snapshots contains two JSON files, and explicitly assert the existence of log.jsonl and HEAD. Remove the misleading top-level entry-count assertion and replace the inaccurate comment with structure-specific validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_track.py`:
- Around line 82-86: Update the assertions following dataset_dirs in the track
test to validate the actual on-disk structure: assert that snapshots contains
two JSON files, and explicitly assert the existence of log.jsonl and HEAD.
Remove the misleading top-level entry-count assertion and replace the inaccurate
comment with structure-specific validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e32979f7-1570-473c-a424-f67f4cdb277b
📒 Files selected for processing (16)
docs/decisions/0001-dataset-snapshot-content.mdsrc/dstrack/_benchmark/_cli.pysrc/dstrack/_benchmark/_profiling.pysrc/dstrack/_benchmark/_runner.pysrc/dstrack/_benchmark/_synthetic.pysrc/dstrack/_track.pysrc/dstrack/errors.pysrc/dstrack/readers/_registry.pysrc/dstrack/snapshot/_builder.pysrc/dstrack/snapshot/_stats.pysrc/dstrack/store.pytests/test_benchmark_cli.pytests/test_reader_registry.pytests/test_snapshot_stats.pytests/test_store.pytests/test_track.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/dstrack/_track.py
- src/dstrack/errors.py
- src/dstrack/snapshot/_builder.py
- src/dstrack/_benchmark/_profiling.py
- src/dstrack/_benchmark/_synthetic.py
- tests/test_benchmark_cli.py
- src/dstrack/_benchmark/_runner.py
- src/dstrack/_benchmark/_cli.py
- src/dstrack/snapshot/_stats.py
- src/dstrack/readers/_registry.py
… instead of overwriting in __init__
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.
Summary
This PR introduces the capability to create a full snapshot into the local store given a database from a file. This functionality can be called from the CLI, or by importing
SnapshotBuilderfromdstrack.snapshot.Changes
Test plan
uv run pytestpassesSummary by CodeRabbit
dstrack trackto compute/store dataset snapshots with lineage continuity, plus--max-rowsto cap in-memory stats.dstrack-benchmarkto generate synthetic CSV benchmarks with environment capture, console reporting, and optional profiling call-tree output.