feat(datasets): add DatasetProfile contract#650
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
6c3fb39 to
adc3f32
Compare
|
Signed-off-by: Albert Cui <albcui@nvidia.com>
albcui
force-pushed
the
albcui/dataset-profile-contract
branch
from
adc3f32 to
f571cbb
Compare
1 participant
Summary by CodeRabbit