feat(datasets): add DatasetProfile contract - #650
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Pydantic stored contract for dataset profiles, including classification, recursive schemas, statistics, partition metadata, sampling, and serialization. Tests cover YAML fixtures, JSON round trips, nested semantic roles, open vocabularies, forward compatibility, and message statistics. ChangesDataset profile contract
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the forward reference instead of stringifying the whole module.
from __future__ import annotationsmakes every annotation in the file a string, which conflicts with the "prefer concrete type hints over string-based type hints" guideline — even though only the two self-referencingFeatureSchema.fields/itemsfields actually need it. Pydantic v2 resolves a quoted self-reference on just those fields without the future import (model_rebuild()still required either way).♻️ Narrow the forward reference to the two self-referencing fields
-from __future__ import annotations - from datetime import datetime @@ - fields: list[FeatureSchema] | None = Field(default=None, description="dtype == struct: named child fields.") - items: FeatureSchema | None = Field(default=None, description="dtype in {list, messages}: element schema.") + fields: list["FeatureSchema"] | None = Field(default=None, description="dtype == struct: named child fields.") + items: "FeatureSchema" | None = Field(default=None, description="dtype in {list, messages}: element schema.")As per coding guidelines,
**/*.pyshould "prefer concrete type hints over string-based type hints," and the plugin-scoped rule reiterates the same preference.Also applies to: 189-190, 361-361
🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py` at line 32, Remove the module-wide `from __future__ import annotations` in `dataset_profile.py` and scope forward references only to the self-referencing `FeatureSchema.fields` and `FeatureSchema.items` annotations using quoted types. Preserve the existing `model_rebuild()` behavior and keep all unrelated annotations concrete.Source: Coding guidelines
packages/nemo_platform_plugin/tests/files/test_dataset_profile.py (1)
171-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_build_profiledoesn't actually exerciseTextStats/NumericStats.
ColumnStats(text=None)andColumnStats(numeric=None)are no-ops (both default toNonealready), so despite the docstring claiming to exercise "every model in the contract,"TextStats/NumericStats/Quantilesare never directly constructed here — only reached indirectly via the YAML fixtures elsewhere in the file.♻️ Populate real stat objects
stats={ - "prompt": ColumnStats(text=None), - "response": ColumnStats(numeric=None), + "prompt": ColumnStats(text=TextStats(chars=Quantiles(p50=10, p95=40, p99=60, max=100))), + "response": ColumnStats(numeric=NumericStats(min=0.0, max=1.0, mean=0.5)), },(Requires importing
TextStats/NumericStatsalongside the existing imports.)🤖 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 `@packages/nemo_platform_plugin/tests/files/test_dataset_profile.py` around lines 171 - 206, Update _build_profile to construct and assign actual TextStats and NumericStats instances in the prompt and response ColumnStats entries, including a populated Quantiles instance where required by those models. Add the corresponding TextStats and NumericStats imports alongside the existing model imports so the hand-built profile directly exercises these contract models.
🤖 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
`@packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py`:
- Line 32: Remove the module-wide `from __future__ import annotations` in
`dataset_profile.py` and scope forward references only to the self-referencing
`FeatureSchema.fields` and `FeatureSchema.items` annotations using quoted types.
Preserve the existing `model_rebuild()` behavior and keep all unrelated
annotations concrete.
In `@packages/nemo_platform_plugin/tests/files/test_dataset_profile.py`:
- Around line 171-206: Update _build_profile to construct and assign actual
TextStats and NumericStats instances in the prompt and response ColumnStats
entries, including a populated Quantiles instance where required by those
models. Add the corresponding TextStats and NumericStats imports alongside the
existing model imports so the hand-built profile directly exercises these
contract models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d067794a-2f37-457d-ae8d-ab85c5354104
📒 Files selected for processing (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.pypackages/nemo_platform_plugin/tests/files/test_dataset_profile.py
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
from
July 13, 2026 16:28
6c3fb39 to
adc3f32
Compare
Contributor
|
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
2 times, most recently
from
July 16, 2026 18:33
f571cbb to
c284981
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py`:
- Around line 193-202: Update the CategoricalStats values field to accept
integer categorical values alongside strings, using the existing contract’s
typing conventions. Preserve the optional default and the requirement that
values represent exhaustive enumerations with distinct_count <= 32.
- Around line 174-182: Enforce the schema invariants in FeatureSchema: allow
fields only for struct dtypes, items only for list or messages dtypes, and
fixed_length only for list dtypes, rejecting incompatible combinations during
validation. In PartitionProfile.stats, validate that every stats key matches a
top-level feature name, covering both the FeatureSchema definition and the stats
validation site.
- Around line 62-65: Constrain the fraction fields in the dataset profile
model—coverage, ends_with_assistant_rate, valid_alternation_rate,
whitespace_ratio, non_ascii_ratio, and null_rate—by adding inclusive lower and
upper bounds of 0 and 1 to each Field declaration. Preserve their existing
defaults and descriptions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03b76d9b-3745-486e-8059-7890e0750d13
📒 Files selected for processing (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.pypackages/nemo_platform_plugin/tests/files/test_dataset_profile.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_platform_plugin/tests/files/test_dataset_profile.py
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
from
July 16, 2026 19:25
c284981 to
6af01ec
Compare
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
from
July 24, 2026 20:02
6af01ec to
4e2e467
Compare
Signed-off-by: Albert Cui <albcui@nvidia.com>
Signed-off-by: Albert Cui <albcui@nvidia.com>
A feature node is either a named-field container or has a single element schema, never both. Enforce that invariant, and only that one: it holds for any dtype, so it costs no forward compatibility. Tying fields / items / fixed_length to specific dtype values would reject a profile written by a newer profiler that added a container dtype, which is what the open vocabulary exists to prevent. Signed-off-by: Albert Cui <albcui@nvidia.com>
Address review comments on fields whose descriptions said when a value was trustworthy without saying what it counted: - rows_total: the whole-fileset denominator rows_scanned is a fraction of; None means unknown, never zero and never an estimate. - files_scanned: a count, not a list; points at SplitProfile.files. - SplitProfile.files: was undescribed; state the exhaustive-and-disjoint split partitioning the PartitionProfile docstring already relies on. - num_examples: counts every file in the split, scanned or not. - roles_seen: record why this is not an enum. Unlike dataset_type or modality, which are vocabularies the profiler picks from, this is verbatim row content — ShareGPT emits human/gpt, Llama tooling emits ipython. Normalizing or dropping an unexpected role would hide the signal a consumer needs to choose a chat template. Signed-off-by: Albert Cui <albcui@nvidia.com>
…ontract
"Inference" collides with LLM inference, which is the wrong association
for a profiler that matches column names and probes content. Replace all
ten occurrences with "detected" / "detection", already the module vocabulary
("new detectors" in the schema-version note, "when nothing was detected"
on semantic_role).
Deliberately not "derived": the file uses that for the measured side --
"the derived row schema", "derived de novo from the data" -- and reusing
it would blur the fact-vs-judgment split the two-layer design rests on.
Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
from
July 27, 2026 17:24
33b39b9 to
59b0b0b
Compare
anubhutivyas
approved these changes
Jul 31, 2026
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
2 participants
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 by CodeRabbit
New Features
Tests