feat(datasets): dataset profiler engine - #658
Conversation
9338534 to
d4241fa
Compare
d4241fa to
1c2076c
Compare
1c2076c to
e41958e
Compare
e41958e to
8a85134
Compare
| for entry in entries: | ||
| by_dir.setdefault(_top_dir(entry.path), []).append(entry) | ||
|
|
||
| if len(by_dir) == 1: |
There was a problem hiding this comment.
what happens when the dir structure is like data/main/*.parquet and data/socratic/*.parquet, it has a single top-level dir data, does it become one default partition mixing two schemas?
There was a problem hiding this comment.
Currently, partitions are based on top-level directories only. So both data/main/*.parquet and data/socratic/*.parquet would be grouped under the data partition.
Note that a partition can be further broken down into splits. Typically, splits are train, val, test etc. Currently, we hard-code this in _CANONICAL_ALIASES, and make a best-effort guess. However, in the future, I'm thinking of extending this to allow the user to specify their own split names in the README.md's YAML front-matter like HF's format: https://huggingface.co/datasets/trl-lib/OpenMathReasoning/blob/main/README.md?code=true#L16-L22
There was a problem hiding this comment.
Updated the PR description with an explanation of how partitions and splits work.
| strings = [value for value in present if isinstance(value, str)] | ||
| if strings: | ||
| text = TextStats(chars=_quantiles([len(value) for value in strings])) | ||
| quality = _text_quality(strings) |
There was a problem hiding this comment.
For a 10GB dataset do we roughly know how much the _text_quality function adds to the runtime?
| else: | ||
| num_rows = result.num_rows | ||
| rows_scanned += result.rows_scanned | ||
| partition_rows.extend(result.rows) |
There was a problem hiding this comment.
Are we worried about OOM on large datasets here?
| stripped = raw_line.strip() | ||
| if not stripped: # tolerate blank lines between records | ||
| continue | ||
| record = json.loads(stripped) |
There was a problem hiding this comment.
first failed entry kills the read, is this intended?
There was a problem hiding this comment.
Good catch, wrapped this with a try/except/continue
8a85134 to
7e1f928
Compare
|
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 dataset profiler plugin with JSONL and Parquet readers, schema and statistical analysis, semantic classification, resilient profile assembly, an expanded dataset-profile contract, and a platform task that publishes ChangesDataset profiling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py (1)
57-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
resolve_splitsmerges distinct non-canonical splits into one"default"split.Line 69's
not any(canonicals.values())check fires whenever NO group name matches a known alias, regardless of how many distinct group names exist. With two files likeabc.parquetandxyz.parquet,groupedhas two keys, neither canonical, so both get merged into a singleResolvedSplit(name="default", ...)(line 70). This discards the distinction between two genuinely different splits and mixes their rows into one split's statistics.The fallback should trigger only when there is exactly one group (the intended case: a single split whose name is not recognized, such as
"shard"after suffix-stripping). With multiple distinct group names, each should stay its ownResolvedSplitwithcanonical=None.🐛 Proposed fix
canonicals = {name: _canonical_for(name) for name in grouped} - if not any(canonicals.values()): + if len(grouped) == 1 and not any(canonicals.values()): return [ResolvedSplit(name="default", canonical=None, entries=list(entries))]Add a regression test covering multiple distinct non-canonical split names (for example
abc.parquetandxyz.parquet) to lock in the corrected behavior.🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py` around lines 57 - 73, Update resolve_splits so the "default" fallback applies only when grouped contains exactly one non-canonical group; preserve separate ResolvedSplit entries with canonical=None for multiple distinct non-canonical names. Add a regression test using files such as abc.parquet and xyz.parquet that verifies two distinct splits remain separate.
🧹 Nitpick comments (2)
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.py (1)
72-78: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFixed-size lists skip
messagesdetection.A
fixed_size_listof{role, content}structs returns dtypelist, but a variable list of the same struct returnsmessages. Chat columns with a constant turn count then classify differently. Consider applying the same check in the fixed-size branch.♻️ Proposed change
if pa.types.is_fixed_size_list(arrow_type): item = _feature_from_arrow("", arrow_type.value_type) - return FeatureSchema(name=name, dtype="list", items=item, fixed_length=arrow_type.list_size) + dtype = "messages" if _is_message_struct(item) else "list" + return FeatureSchema(name=name, dtype=dtype, items=item, fixed_length=arrow_type.list_size)🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.py` around lines 72 - 78, Update the fixed-size list branch in _feature_from_arrow to derive dtype using _is_message_struct(item), matching the variable-list branch. Fixed-size lists whose items are message structs should return dtype "messages", while other fixed-size lists must remain dtype "list".plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py (1)
55-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd root-containment check in
open().
open()joinspathontoself._rootwithout validating containment. Today,pathalways comes fromlist_files(), so this is not exploitable yet. The docstring states otherFileSourceimplementations will share this same two-method contract. Add a containment check now, so the contract stays safe as new sources adopt it.🔒️ Proposed fix
def open(self, path: str) -> BinaryIO: - return open(self._root / path, "rb") + target = (self._root / path).resolve() + if not target.is_relative_to(self._root.resolve()): + raise ValueError(f"{path!r} escapes the source root") + return open(target, "rb")
Path.is_relative_torequires Python 3.9+. Confirm the plugin's minimum supported Python version before applying.🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py` around lines 55 - 56, Update FileSource.open to resolve the candidate path and verify it remains within self._root before opening it; reject paths escaping the root while preserving valid file access. Use Path.is_relative_to only if the plugin’s minimum Python version supports it, otherwise implement the equivalent containment check with compatible pathlib operations.Source: Linters/SAST tools
🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.py`:
- Around line 142-143: Update the schema handling around arrow_schema in the
profiler pipeline to merge schemas from every readable shard, including later
results instead of retaining only the first schema. Use pa.unify_schemas with
appropriate error handling that preserves the existing first-schema fallback
when schemas are incompatible, ensuring columns unique to later shards remain
available to derive_features and stats.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.py`:
- Around line 47-57: Update _load_builtin_readers so _builtins_loaded is
assigned True only after the jsonl and parquet imports complete successfully;
leave it false when the import raises, allowing subsequent get_reader() calls to
retry and surface the original import error.
---
Outside diff comments:
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py`:
- Around line 57-73: Update resolve_splits so the "default" fallback applies
only when grouped contains exactly one non-canonical group; preserve separate
ResolvedSplit entries with canonical=None for multiple distinct non-canonical
names. Add a regression test using files such as abc.parquet and xyz.parquet
that verifies two distinct splits remain separate.
---
Nitpick comments:
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py`:
- Around line 55-56: Update FileSource.open to resolve the candidate path and
verify it remains within self._root before opening it; reject paths escaping the
root while preserving valid file access. Use Path.is_relative_to only if the
plugin’s minimum Python version supports it, otherwise implement the equivalent
containment check with compatible pathlib operations.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.py`:
- Around line 72-78: Update the fixed-size list branch in _feature_from_arrow to
derive dtype using _is_message_struct(item), matching the variable-list branch.
Fixed-size lists whose items are message structs should return dtype "messages",
while other fixed-size lists must remain dtype "list".
🪄 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: ecfc9dde-ef8c-4755-a209-108f3d429acc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
plugins/nemo-datasets/pyproject.tomlplugins/nemo-datasets/src/nemo_datasets_plugin/cli.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/digest.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/partition.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/parquet.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.pyplugins/nemo-datasets/tests/test_classify.pyplugins/nemo-datasets/tests/test_cli.pyplugins/nemo-datasets/tests/test_pipeline.pyplugins/nemo-datasets/tests/test_readers.pyplugins/nemo-datasets/tests/test_schema.pyplugins/nemo-datasets/tests/test_stats.pypyproject.tomlpytest.ini
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.py (1)
151-152: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_count_matchesstill loops in Python; the comment claims C-level scanning.
finditeryields a match object per character hit, and thesumloop runs in Python. For single-character patterns you can count without per-match objects.♻️ Cheaper count
def _count_matches(pattern: re.Pattern[str], text: str) -> int: - return sum(1 for _ in pattern.finditer(text)) + return len(pattern.findall(text))🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.py` around lines 151 - 152, Update _count_matches to use a C-level counting operation for single-character patterns instead of iterating over pattern.finditer and creating one match object per hit; preserve the existing count result for all supported patterns.plugins/nemo-datasets/src/nemo_datasets_plugin/cli.py (2)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the help text tied to the pipeline default.
run_profile(source)uses the pipeline default, but the option help hard-codes1000. If the pipeline default changes,nemo datasets profile --helpwill report incorrect behavior. Use a shared lightweight constant or remove the numeric value from the help text.🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/cli.py` around lines 32 - 43, Update the rows_per_file option help in get_cli so it does not hard-code 1000; either reference a shared lightweight pipeline-default constant or describe the default without a numeric value, while preserving the existing 0-means-all-rows behavior.
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd CLI coverage for row-cap and output behavior.
Test omitted
--rows-per-fileand--rows-per-file 0separately. The omitted option must preserve the pipeline default. Zero must passrow_cap=Nonefor exhaustive reading. Also assert JSON and YAML output. The existing pipeline tests callprofiledirectly and cannot catch incorrect CLI wiring.🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/cli.py` around lines 54 - 63, Add CLI-level tests for the command containing the run_profile call, covering omitted --rows-per-file and an explicit zero separately; verify omission preserves the pipeline default while zero passes row_cap=None. Also exercise both output modes and assert the emitted JSON and YAML content, rather than relying on direct profile tests.
🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/cli.py`:
- Around line 32-43: Update the rows_per_file option help in get_cli so it does
not hard-code 1000; either reference a shared lightweight pipeline-default
constant or describe the default without a numeric value, while preserving the
existing 0-means-all-rows behavior.
- Around line 54-63: Add CLI-level tests for the command containing the
run_profile call, covering omitted --rows-per-file and an explicit zero
separately; verify omission preserves the pipeline default while zero passes
row_cap=None. Also exercise both output modes and assert the emitted JSON and
YAML content, rather than relying on direct profile tests.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.py`:
- Around line 151-152: Update _count_matches to use a C-level counting operation
for single-character patterns instead of iterating over pattern.finditer and
creating one match object per hit; preserve the existing count result for all
supported patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 168762dc-5f88-4ef5-b5c5-7fa9bc9735d1
📒 Files selected for processing (15)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.pyplugins/nemo-datasets/src/nemo_datasets_plugin/cli.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/partition.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.pyplugins/nemo-datasets/tests/test_classify.pyplugins/nemo-datasets/tests/test_pipeline.pyplugins/nemo-datasets/tests/test_readers.pyplugins/nemo-datasets/tests/test_schema.pyplugins/nemo-datasets/tests/test_stats.py
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/partition.py
- plugins/nemo-datasets/tests/test_classify.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.py
- plugins/nemo-datasets/tests/test_schema.py
2017d23 to
149e1f5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py (1)
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a concrete step-config type.
Bare
dicthides the configuration value contract. Usedict[str, object]for raw JSON, then validate into a typed config model.
plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py#L101-L101: type the_build_source()config input.plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py#L110-L110: type the_resolve_row_budget()config input.plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py#L134-L134: type the_resolve_column_roles()config input.plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py#L147-L147: type and validate the_load_step_config()result.plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py#L155-L155: type the_required_config()config input.As per coding guidelines, “Prefer concrete type hints over string-based ones.”
🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py` at line 101, Replace bare dict annotations in _build_source(), _resolve_row_budget(), _resolve_column_roles(), and _required_config() with dict[str, object]. Update _load_step_config() to return the typed configuration model after validating the raw JSON configuration, using concrete type hints throughout.Source: Coding guidelines
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py (2)
206-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the missing return annotation.
_messages_statsis the only function here without one.MessageStats | Noneis concrete and available at runtime fromnemo_platform_plugin.files.dataset_profile.♻️ Proposed change
-def _messages_stats(features: list[FeatureSchema], stats: dict[str, ColumnStats]): +def _messages_stats(features: list[FeatureSchema], stats: dict[str, ColumnStats]) -> MessageStats | None:Add
MessageStatsto the existing import block.Based on coding guidelines: "Prefer concrete type hints over string-based ones, and do not import those types only under
TYPE_CHECKING; use regular imports when possible."🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py` around lines 206 - 212, Add the runtime MessageStats import from nemo_platform_plugin.files.dataset_profile and annotate _messages_stats with MessageStats | None, preserving its existing return behavior.Source: Coding guidelines
328-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the same row type as
stats.
stats.derive_probesdeclaresrows: list[dict[str, Any]]. Here and inclassify(Line 364) the rows arelist[dict], so the element type is unparameterized across a module boundary that passes the same value.♻️ Proposed change
def _implicit_prompt_evidence( - features: list[FeatureSchema], probes: dict[str, ColumnProbes], rows: list[dict] + features: list[FeatureSchema], probes: dict[str, ColumnProbes], rows: list[dict[str, Any]] ) -> Evidence | None:Import
Anyfromtypingand apply the same change toclassify.Based on coding guidelines: "Prefer concrete type hints over string-based ones."
🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py` around lines 328 - 331, Update the rows parameter annotations in _implicit_prompt_evidence and classify to use list[dict[str, Any]], importing Any from typing as needed. Keep the existing row-processing behavior unchanged and match stats.derive_probes’ concrete row type across the module boundary.Source: Coding guidelines
plugins/nemo-datasets/tests/test_stats.py (2)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the actual behaviour instead of accepting either.
For
float64with all values non-finite,_column_statsstill recordscategoricalfrom_cardinality, so the column is returned and onlynumericisNone. The disjunction lets a future regression that drops the column entirely pass.💚 Proposed change
def test_numeric_all_non_finite_yields_no_numeric_summary(): stats = derive_stats([_feature("n", "float64")], _rows("n", [float("nan"), float("inf")])) - assert stats.get("n") is None or stats["n"].numeric is None + assert stats["n"].numeric is None🤖 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 `@plugins/nemo-datasets/tests/test_stats.py` around lines 83 - 85, Update test_numeric_all_non_finite_yields_no_numeric_summary to assert that stats["n"] exists and its numeric field is None, while preserving the existing float64 all-non-finite fixture.
159-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for duplicate column names.
derive_statsandderive_probesboth document that a duplicate Parquet field name resolves to first-wins, and both state the two must agree. Neither behaviour is tested.💚 Proposed test
def test_duplicate_field_names_resolve_to_the_first(): # Parquet permits duplicate field names; stats and probes must agree on which one wins. features = [_feature("t", "string"), _feature("t", "int64")] rows = _rows("t", ["a", "bb"]) stats = derive_stats(features, rows) assert stats["t"].text is not None # the string feature, not the int64 one assert set(derive_probes(features, rows)) == {"t"}🤖 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 `@plugins/nemo-datasets/tests/test_stats.py` around lines 159 - 168, Add a test near test_unmeasured_dtypes_are_omitted named test_duplicate_field_names_resolve_to_the_first that supplies duplicate “t” features with string first and int64 second, verifies derive_stats uses the string feature by asserting text is populated, and confirms derive_probes returns only the single “t” key.plugins/nemo-datasets/tests/test_classify.py (1)
358-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a declared role the vocabulary does not contain.
The hint tests cover a dtype mismatch and a fallback to detection. They do not cover a misspelled role name, which
_dtype_allowscurrently accepts (see the comment onclassify.pyLines 112-132). Add the test with the vocabulary fix.💚 Proposed test
def test_a_hint_naming_an_unknown_role_is_rejected(): # A typo in the role name is as costly as a typo in the column name, and must not be stored. features = [_f("q", "string")] result = classify(features, {}, column_roles={"q": "prmpt"}) assert features[0].semantic_role is None assert [e.kind for e in result.evidence if e.kind == "user_hint"] == ["user_hint"]🤖 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 `@plugins/nemo-datasets/tests/test_classify.py` around lines 358 - 375, Add a test alongside the existing hint tests for an unknown declared role, using a misspelled role such as “prmpt” on a string feature; assert that no semantic role is assigned and exactly one user_hint rejection is recorded. Update the role-vocabulary validation used by classify/_dtype_allows so unknown role names are rejected before dtype checks, while preserving fallback detection behavior for valid roles with incompatible dtypes.
🤖 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 `@plugins/nemo-datasets/pyproject.toml`:
- Around line 17-19: Update the plugin configuration in pyproject.toml to
register the required nemo datasets profile command through the nemo.cli entry
point, replacing the deliberate omission. Ensure the registration invokes the
existing profiling task or CLI handler, and add an integration test that
executes nemo datasets profile and verifies it reaches the expected profile
behavior.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py`:
- Around line 112-132: Update _dtype_allows and the declared-role handling in
_assign_roles to validate role names against the canonical role vocabulary
before accepting them. Unknown roles such as “prmpt” must be rejected rather
than returning True or being stored in FeatureSchema.semantic_role; record the
rejection as Evidence using the existing dtype-mismatch reporting path, while
preserving valid-role behavior.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py`:
- Around line 47-51: Update FileSource.list_files and the file-opening path in
FileSource so entries cannot escape the source root via symlinks. Replace the
current path.is_file() filter with logic that excludes symlinks and verifies
each candidate resolves inside self._root before it is listed or opened, and
make the open path use a no-follow, race-safe approach in FileSource. Add a test
that creates a symlink inside the root pointing to a file outside the root and
confirms it is not returned or opened.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.py`:
- Around line 244-246: Update the fallback PartitionClassification construction
in pipeline.py to pass candidates=["unknown"] alongside dataset_type="unknown".
In
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py,
add a model_validator for the relevant classification model that rejects
non-empty candidates when candidates[0] differs from dataset_type, enforcing the
documented invariant.
- Around line 4-16: Update the top-level module docstring to remove “content
digest” from the structural envelope description and replace the stale row_cap
discussion with the current row_budget behavior, including that the budget is
divided across files in a partition and that row_budget=None performs an
exhaustive scan.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.py`:
- Around line 111-122: Update the numeric processing in the _is_numeric branch
to convert values through a finite-float helper that catches OverflowError and
rejects NaN or infinities, returning None for unrepresentable values. Use that
helper when building numbers so oversized integers are skipped without aborting
profiling, while preserving NumericStats and _cardinality behavior for valid
values.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py`:
- Around line 123-131: Update the row-budget validation around requested and
budget so only JSON integer values are accepted before conversion, explicitly
rejecting fractional values and bool instances; then preserve the existing
handling of zero as None and negative budgets as errors. Add tests covering
fractional and boolean row_budget inputs.
---
Nitpick comments:
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py`:
- Around line 206-212: Add the runtime MessageStats import from
nemo_platform_plugin.files.dataset_profile and annotate _messages_stats with
MessageStats | None, preserving its existing return behavior.
- Around line 328-331: Update the rows parameter annotations in
_implicit_prompt_evidence and classify to use list[dict[str, Any]], importing
Any from typing as needed. Keep the existing row-processing behavior unchanged
and match stats.derive_probes’ concrete row type across the module boundary.
In `@plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.py`:
- Line 101: Replace bare dict annotations in _build_source(),
_resolve_row_budget(), _resolve_column_roles(), and _required_config() with
dict[str, object]. Update _load_step_config() to return the typed configuration
model after validating the raw JSON configuration, using concrete type hints
throughout.
In `@plugins/nemo-datasets/tests/test_classify.py`:
- Around line 358-375: Add a test alongside the existing hint tests for an
unknown declared role, using a misspelled role such as “prmpt” on a string
feature; assert that no semantic role is assigned and exactly one user_hint
rejection is recorded. Update the role-vocabulary validation used by
classify/_dtype_allows so unknown role names are rejected before dtype checks,
while preserving fallback detection behavior for valid roles with incompatible
dtypes.
In `@plugins/nemo-datasets/tests/test_stats.py`:
- Around line 83-85: Update
test_numeric_all_non_finite_yields_no_numeric_summary to assert that stats["n"]
exists and its numeric field is None, while preserving the existing float64
all-non-finite fixture.
- Around line 159-168: Add a test near test_unmeasured_dtypes_are_omitted named
test_duplicate_field_names_resolve_to_the_first that supplies duplicate “t”
features with string first and int64 second, verifies derive_stats uses the
string feature by asserting text is populated, and confirms derive_probes
returns only the single “t” key.
🪄 Autofix
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: a59a5382-d6e9-4140-81a3-271ab0c2d64c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.pypackages/nemo_platform_plugin/tests/files/test_dataset_profile.pyplugins/nemo-datasets/pyproject.tomlplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/partition.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/parquet.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.pyplugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/__main__.pyplugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.pyplugins/nemo-datasets/tests/test_classify.pyplugins/nemo-datasets/tests/test_pipeline.pyplugins/nemo-datasets/tests/test_profile_task.pyplugins/nemo-datasets/tests/test_readers.pyplugins/nemo-datasets/tests/test_schema.pyplugins/nemo-datasets/tests/test_stats.pypyproject.tomlpytest.ini
🚧 Files skipped from review as they are similar to previous changes (9)
- pytest.ini
- pyproject.toml
- plugins/nemo-datasets/tests/test_readers.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/parquet.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.py
- plugins/nemo-datasets/tests/test_schema.py
| def list_files(self) -> list[FileEntry]: | ||
| entries = [ | ||
| FileEntry(path=path.relative_to(self._root).as_posix(), size_bytes=path.stat().st_size) | ||
| for path in sorted(self._root.rglob("*")) | ||
| if path.is_file() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py'
ast-grep outline "$file" --view compact
cat -n "$file" | sed -n '1,90p'
rg -n -A12 -B8 'class LocalFileSource|def open|def list_files' plugins/nemo-datasets/src plugins/nemo-datasets/testsRepository: NVIDIA-NeMo/nemo-platform
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -eu
file='plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py'
ast-grep outline "$file" --view signatures
cat -n "$file" | sed -n '1,90p'
rg -n -A12 -B8 'class LocalFileSource|def open|def list_files' plugins/nemo-datasets/src plugins/nemo-datasets/testsRepository: NVIDIA-NeMo/nemo-platform
Length of output: 6370
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
source_path = Path("plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py")
tree = ast.parse(source_path.read_text())
local = next(
node for node in tree.body
if isinstance(node, ast.ClassDef) and node.name == "LocalFileSource"
)
methods = {
node.name: node
for node in local.body
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
}
listing = ast.unparse(methods["list_files"])
opening = ast.unparse(methods["open"])
print("list_files:", listing)
print("open:", opening)
print("follows_symlinks_via_is_file:", ".is_file(" in listing)
print("uses_resolve_containment:", ".resolve(" in listing or ".resolve(" in opening)
print("rejects_symlinks:", "is_symlink" in listing or "O_NOFOLLOW" in opening)
print("opens_joined_path:", "self._root / path" in opening)
PYRepository: NVIDIA-NeMo/nemo-platform
Length of output: 600
Path Traversal (CWE-59)
Exploitability: Moderate
Reachability path
● Entry
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.py:71
profile: Profile the dataset behind ``source`` into a ``DatasetProfile``. ``row_budget`` bounds how many rows each *partition* reads in total, div…
│
▼
● Sink
plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py
Reject symlink escapes from the source root. path.is_file() includes symlink targets, and open(self._root / path, "rb") follows them. Exclude symlinks and enforce resolved-root containment before opening. Use a no-follow, race-safe open when the directory can change. Add a test for a symlink to a file outside the root.
🤖 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 `@plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.py`
around lines 47 - 51, Update FileSource.list_files and the file-opening path in
FileSource so entries cannot escape the source root via symlinks. Replace the
current path.is_file() filter with logic that excludes symlinks and verifies
each candidate resolves inside self._root before it is listed or opened, and
make the open path use a no-follow, race-safe approach in FileSource. Add a test
that creates a symlink inside the root pointing to a file outside the root and
confirms it is not returned or opened.
Source: Linters/SAST tools
| return file_format | ||
|
|
||
|
|
||
| def profile( |
There was a problem hiding this comment.
start here: this is the main entrypoint
149e1f5 to
db0df00
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-datasets/tests/test_profile_task.py (1)
135-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the failure cause, not just the exit code.
run()catches everyExceptionand returns 1. These tests pass even if the task fails for an unrelated reason. Call_resolve_row_budgetand_resolve_column_rolesdirectly withpytest.raises(ValueError), or assert the logged message withcaplog. Add cases for fractional and booleanrow_budget, which the currentint()coercion accepts.🤖 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 `@plugins/nemo-datasets/tests/test_profile_task.py` around lines 135 - 144, Strengthen the validation tests by calling _resolve_row_budget and _resolve_column_roles directly and asserting pytest.raises(ValueError), rather than only checking run()’s generic exit code. Add row_budget cases for fractional and boolean values, ensuring these are rejected instead of accepted through int() coercion; use caplog only if direct resolver coverage is not feasible.
🤖 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 99-117: Update the measurement-failure path in _measure to emit
candidates containing "unknown" alongside dataset_type="unknown". Add a model
validator for the dataset profile model that rejects empty candidates and
requires candidates[0] to equal dataset_type, while preserving the documented
ordering.
---
Nitpick comments:
In `@plugins/nemo-datasets/tests/test_profile_task.py`:
- Around line 135-144: Strengthen the validation tests by calling
_resolve_row_budget and _resolve_column_roles directly and asserting
pytest.raises(ValueError), rather than only checking run()’s generic exit code.
Add row_budget cases for fractional and boolean values, ensuring these are
rejected instead of accepted through int() coercion; use caplog only if direct
resolver coverage is not feasible.
🪄 Autofix
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: b6b5a0ed-cf1f-4aa2-892e-e03bc30cf3fe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.pypackages/nemo_platform_plugin/tests/files/test_dataset_profile.pyplugins/nemo-datasets/pyproject.tomlplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/file_source.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/partition.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/parquet.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/schema.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.pyplugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.pyplugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/__main__.pyplugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/run.pyplugins/nemo-datasets/tests/test_classify.pyplugins/nemo-datasets/tests/test_pipeline.pyplugins/nemo-datasets/tests/test_profile_task.pyplugins/nemo-datasets/tests/test_readers.pyplugins/nemo-datasets/tests/test_schema.pyplugins/nemo-datasets/tests/test_stats.pypyproject.tomlpytest.ini
🚧 Files skipped from review as they are similar to previous changes (14)
- pytest.ini
- plugins/nemo-datasets/pyproject.toml
- plugins/nemo-datasets/src/nemo_datasets_plugin/tasks/profile/main.py
- plugins/nemo-datasets/tests/test_classify.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/parquet.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/splits.py
- plugins/nemo-datasets/tests/test_readers.py
- plugins/nemo-datasets/tests/test_stats.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/jsonl.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/readers/base.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/pipeline.py
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/classify.py
- pyproject.toml
- plugins/nemo-datasets/src/nemo_datasets_plugin/profiler/stats.py
| dataset_type: str = Field( | ||
| description=( | ||
| "Dataset-type vocabulary (prompt_completion, preference_pair, ...). A SUMMARY, not the " | ||
| "basis for a decision — it is the most specific single structure the roles satisfy, and a " | ||
| "dataset routinely satisfies several. The `semantic_role` markers are what a consumer " | ||
| "should match on; `candidates` lists everything this one is a projection of." | ||
| ), | ||
| ) | ||
| candidates: list[str] = Field( | ||
| default_factory=list, | ||
| description=( | ||
| "Every dataset type the assigned roles satisfy, most specific first, so " | ||
| "`candidates[0] == dataset_type`. prompt + completion + score + label is genuinely both " | ||
| "scored_response and unpaired_preference; reporting only the first made rule order an " | ||
| "invisible tie-break and hid that the data supports more than one use. Deliberately not a " | ||
| 'capability list ("supports DPO") — trainer requirements shift and differ per framework, ' | ||
| "so that mapping belongs in the consumer, computed from the roles." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the classification candidate invariant.
candidates defaults to [], but _measure emits dataset_type="unknown" without candidates after a measurement failure. This profile contradicts candidates[0] == dataset_type and can fail consumers that use the documented primary candidate.
Emit candidates=["unknown"] on that path. Add a model validator that requires a non-empty list with candidates[0] == dataset_type.
🤖 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`
around lines 99 - 117, Update the measurement-failure path in _measure to emit
candidates containing "unknown" alongside dataset_type="unknown". Add a model
validator for the dataset profile model that rejects empty candidates and
requires candidates[0] to equal dataset_type, while preserving the documented
ordering.
Phase 3, third step, and the reservoir the spec called for is not what shipped. Exact quantiles need every length kept and sorted, which is a list that grows with the dataset. A reservoir of sampled lengths bounds that, but buys the bound with an RNG -- and so with `SamplingInfo.seed` back in the contract after it was deleted on the grounds that the profiler makes no random choices. Counting into fixed buckets bounds it with neither. The two put their error in different places, and that is the whole argument. A reservoir sees *some* rows exactly: its error is in which rows it kept, which is probabilistic and only shrinks with the sample size. A histogram sees *every* row imprecisely: its error is in how finely each value was recorded, which is a hard bound of half a bucket width whatever the data does. Rounding the value is the cheap error to accept, because the number is read to pick a sequence budget and gets rounded to a power of two by whoever reads it. Lengths below 32 get a counter each and stay exact. Above that each octave is cut into 32 slices, so a bucket spans a fixed 1/32 of its value and the midpoint lands within ~1.6%. Measured against exact quantiles on the real shards, worst error over fifteen estimates is 1.5%, and the bucket/bounds round trip is verified from 0 to 33.5M. Midpoint, not the bucket's low edge. The edge sits systematically under the truth -- every estimate in the first measurement came out low -- and centring roughly halves the average error. Clamped to `max`, which is tracked separately and stays exact: a p99 above the largest value present would be nonsense, and `max` is the one number here a reader may treat as a hard bound. A messages column is now fully O(1): its two histograms cost 280 bytes and 22 KB over 43,835 rows, against ~342 KB for a retained list of lengths or ~800 KB for a 100k reservoir. A string column has one term left -- it still retains its strings, because the quality stride needs the column's row count to place its sample and that is not known until the last batch. Summing the parquet footers before folding removes it, and that is the next step. Contract: p50/p95/p99 are documented as estimates and `max` as exact. `p95` is kept -- I proposed dropping it and it was not part of what was agreed. `stats_complete` now says plainly that it speaks to rows read rather than to every number being exact, which replaces the narrower TextQuality-only carve-out from Phase 2. Also deleted `_message_stats`, superseded by MessageAccumulator two commits ago and still calling `_quantiles`. Tests never caught it because nothing called it; ruff and ty did. Signed-off-by: Albert Cui <albcui@nvidia.com>
Phase 3, fourth step. `quote_enumerations` and `classify` were the last two things between a measured partition and a materialised one, and both were reaching back to the rows for something already computed. `quote_enumerations` rescanned every row to rebuild a distinct set the vocabulary had just built and thrown away. `_Vocabulary` now hands out what it kept and the pass reads that. `measure_columns` grew a named result to carry it -- three positional returns was already the limit, and the vocabularies are not part of the stored profile, so a tuple was the wrong shape for them. `classify` took rows for two reasons. It derived probes when handed none, which only the tests ever used; absent probes now read as "nothing was measured", which is the honest reading and never "nothing is there". And it ran the chosen/rejected shared-prefix check, which is the one probe that compares two columns of the same row against each other and so can live neither on a column accumulator nor in a per-column probe. That becomes `PrefixPairFold`: two counters, folded over the same batches, resolved by column *name* because the fold runs before classification has assigned any roles -- the same inversion the content probes made when they stopped being role-gated. `derive_probes` goes with it, having lost its only production caller for the same reason `derive_stats` did two commits ago. The tests keep a two-line local view. The prefix threshold, inlined as `16`, is now named. Nothing in the measure stage reads a row any more except the two folds themselves, which is what the next step needs: batches arrive, both folds consume them, and schema, stats, probes, classification and quoting all follow from what was folded. Behaviour is unchanged and checked as such: the demo profile is byte-identical to the one HEAD produces, diffed against a stashed build of the same tree. Signed-off-by: Albert Cui <albcui@nvidia.com>
Phase 3, last step. A partition whose files all declare a schema is now measured batch by batch and never held. The footers make it possible. A parquet file declares its schema and its exact row count there, so one seek per file establishes the partition's whole shape before a row is parsed -- which is precisely what a fold cannot otherwise have. The accumulators have to exist before the first batch, and the quality stride has to be placed before the column it strides has been seen. Both come out of `peek()`. Result on a real shard: 65.1 MB to profile 21,362 rows exhaustively becomes 10.4 MB, and 19.9 MB to profile 6,038 becomes 10.6 MB. Reading everything now costs what reading some of it costs. `row_budget` stops being a memory guard and becomes a limit on work. A string accumulator no longer retains its strings. With the row count known it places the quality stride up front and measures or skips each string as it goes by, holding counters instead. That was the last term sized by the column. Partitions without a declared schema still materialise, because the schema has to be inferred from the rows and the rows have to be kept until it has been. That is line-delimited formats; folding them needs accumulators created lazily as columns appear, which is Phase 4. Both readers grew `peek()` and `batches()` regardless, and the jsonl parse loop is now shared between `read()` and `batches()` so the two cannot drift on what counts as a row -- a blank line, a stray scalar and a truncated line are three different things and only one is an error. Behaviour is unchanged, checked two ways. The demo profile is byte-identical to HEAD's, diffed against a stashed build. And a test profiles the same 200 rows as parquet and as jsonl -- one folded, one materialised -- and asserts the stats, features and classification match, so the batch size cannot leak into the numbers. Three tests had been passing for the wrong reason: they monkeypatched `measure_columns` to force a measurement failure, which the fold path does not call. One of them still passed because its poisoned partition classified as `unknown` anyway. All three now patch `classify`, which both paths go through. Also caught by ruff and ty rather than by tests: `get_reader` had ended up outside the per-file guard, so a format with no registered reader would have aborted the partition instead of being reported as a FileError. Signed-off-by: Albert Cui <albcui@nvidia.com>
Phase 4, first step: §5 of the streaming spec. Two structures were fed straight from row content with nothing stopping them, and neither needed a bound before because the row budget was one by accident. `_features_from_rows` builds a key union across every row, so a malformed file whose rows carry unique keys mints a column per row -- and since Phase 3, an accumulator per column with it. `MAX_COLUMNS = 4096` stops that. The truncation is reported rather than silent: a profile that described 4,096 of a file's columns as though they were all of them is worse than one that failed, because the reader has no way to tell a wide table from a broken one. Applied to declared schemas too, which are bounded by their file but not by anything sensible. `roles_seen` is worse in one respect: membership is checked against the list, so an unbounded one is quadratic as well as unbounded. `_MAX_ROLES_SEEN = 64`. That truncation is silent, and deliberately so -- the list exists so a reader can pick a chat template, and a column with more than sixty-four distinct roles is not a chat column, which the first few dozen already say. The contract says it is bounded. Reachable today, not hypothetically: with `row_budget=None` there is nothing else holding either of them down, and Phase 3 made an unbounded read the affordable default rather than the expensive exception. Signed-off-by: Albert Cui <albcui@nvidia.com>
Phase 4. Every partition now folds; nothing is materialised. The blocker was never lazy column creation, which §7 of the spec assumed. It was dtypes. An accumulator is chosen *by* dtype, and for an inferred schema the dtype is a whole-column decision -- observed types unioned, a disagreement widened to `json` -- so the choice cannot be made while making it still matters. Batch 1 says string, batch 50 says int, and a StringAccumulator has been folding the wrong thing for forty-nine batches. Deferring the choice is the only resolution that neither reads the data twice nor decides from a prefix and hopes. `DeferredAccumulator` measures every shape at once and picks the answer at the end. It costs nothing extra per value -- a string only ever reaches the string state -- and what it costs is four bounded structures per column instead of one. Alongside it `SchemaFold`, which is `_infer_feature` written incrementally. That turned out to be transcription rather than invention: the function was already a set union over observed types, a union over a struct's child keys, and a recursion over a list's flattened elements, all of which are state proportional to the schema and not to the row count. Checked against the original across fourteen shapes -- widening, mixed, nested structs, both chat spellings, empty lists -- at four chunkings each, with no mismatches. Columns are created on first sight and back-filled with the rows they were absent for, which is a pair of additions rather than a pass, and is what makes the result identical to inferring the schema first and measuring second: a row without the key genuinely holds a null for it. The quality stride no longer needs a row count either, so a string column retains nothing on any path. With a footer the stride is fixed and the sample spread evenly; without one it starts at one and doubles as the sample fills, with each sampled row weighted by the stride it stood for -- Horvitz-Thompson, so the estimate stays unbiased rather than weighted toward the head where sampling was densest. That was the last O(rows) term anywhere in the fold: a 60,000-row jsonl went from 9.6 MB to 1.0 MB, and now costs less exhaustively than it did budgeted. Two bugs found on the way, neither by the tests: `_resolve_scalar` ended in `dtypes.pop()`, which mutates its argument. Harmless while every caller passed a throwaway set; `SchemaFold` passes the one it is still using, so the first `finalize()` emptied it and the second resolved the same column to `json`. `batches()` had no way to report a line it could not parse, so a partially read jsonl folded silently and looked complete -- `read()` reported it, and the fold path does not call `read()`. A generator cannot return that: by the time it knows, the caller has consumed everything it yielded. It takes an `errors` list instead. `_measure` is deleted, having lost its last caller and been quietly broken since `PrefixPairFold` stopped taking a schema. `PrefixPairFold` now resolves its two columns off each row, which is what lets it run over a partition whose columns are not known yet. The test that covered `_measure`'s inference is now an end-to-end one against the pipeline, where that behaviour actually lives. Parquet profiles are byte-identical to Phase 3's. Signed-off-by: Albert Cui <albcui@nvidia.com>
… means Phase 5, the last of the streaming spec. The engine can finally honour the contract change, so it lands. `DEFAULT_ROW_BUDGET` is now None. The budget existed to keep a materialised partition off the heap, and nothing is materialised: a fold's memory is flat in rows, so an exhaustive read costs what a short one costs. The default should not answer the question worse than it can be answered. The demo now reads all 67,551 rows at 11.5 MB peak, both partitions `rows_complete`. `stats_complete` becomes `rows_complete`, which is what it measured all along. The old name promised more than it delivered -- `Quantiles` and `TextQuality` are estimates by construction however much was read, and each says so itself. Whether a number is exact is a property of that number; this says only whether anything was missed on the way in. `SamplingInfo.row_budget` is dropped. It is an input, not a finding, and the finding is already there: `rows_scanned` against `rows_present` says a read was short, and the only other cause -- a file that failed -- is named on `file_errors`. The parameter survives on `profile()` and in the step config for a caller who wants a shorter run. Two deviations from the spec, both because the design moved under it. `rows_measured` is not added. §11 conceived it as the denominator behind an estimate's confidence, sized `min(reservoir, rows_scanned)`. There is no reservoir: quantiles read off a histogram over every scanned row, and the quality sample is a per-column stride. There is no single dataset-level number left for the field to hold, and inventing one would be worse than the absence. `_per_file_cap` and `MIN_ROWS_PER_FILE` survive, which Q3 guessed they might not. They look like budget-splitting machinery invented for the memory problem, but dividing a budget across files was always about *coverage*: reading files in order until a total ran out leaves the later ones unopened, which hides the columns only they witness. Same hole, different route. Memory was never what that arithmetic was protecting. Correction to the spec's runtime projection while I am here. §2 put an exhaustive OpenMathReasoning at ~3 min on the strength of a 450 M chars/s "cheap path", which was measured on a stripped loop -- no probes, no vocabulary, no histogram. The real fold runs at 13 M chars/s below the quality stride's threshold and 22 M above it, which puts that dataset nearer 50 minutes. Memory was the goal and memory is flat; runtime scaling with the dataset was accepted going in. But the number in the spec was wrong and is now the measured one. Signed-off-by: Albert Cui <albcui@nvidia.com>
… what it read Five findings from a review pass over the branch. Two were defects the tests did not cover, and one of them contradicts a claim I had written into a comment on the strength of too little measurement. **The quality sample aliased against periodic data.** It was taken at an even step, and data is periodic more often than it looks -- a set that round-robins over ten sources, or carries k responses per prompt, is periodic by construction. When the step shares a factor with that period it samples one phase and only that phase. At the shipped constants: 500,000 rows with every tenth corrupt gives a step of ten, and a reported repetition score of 1.000 against a truth of 0.100. Not noise. The wrong answer, tenfold. I had measured this before and cleared it. One dataset, HelpSteer2, period two, ≤8% drift -- and I wrote "does not buy a block-sampling scheme to avoid" into the comment. One sample whose period happened not to align with the step, generalised. Block sampling is exactly the fix I dismissed. The sample is now contiguous blocks of 512 rows, spaced evenly. A block longer than the period sees every phase of it, whatever the period is, and costs the same. Measured across periods 2, 5, 10, 25 and 100 at 500,000 rows: every one within 2%. The unknown-row-count path doubles the cycle rather than the step, so blocks stay whole as it thins, and the Horvitz-Thompson weight comes along unchanged. **A file that failed partway was counted as unread.** `rows_scanned` and `files_read` were incremented after the batch loop, which was right when a read was all-or-nothing and wrong once it streamed: a fold cannot give rows back, so batches already folded are in the statistics whatever happens next. A failure on the third batch reported `rows_scanned: 0` and `files_read: 0` beside stats built from 2,048 rows. Both now count what was actually consumed. **Three smaller things.** The stored contract pointed twice at `SplitProfile.files`, a field deleted several phases ago -- that is the docstring a consumer reads to understand the type. The relational prefix probe ran outside the per-column guard, so a failure in it would have surfaced as a `FileError`, collapsing the one distinction the two failure domains exist to keep. And `_PartitionFolds._declared` was assigned but never read, with `_per_file_cap` computed twice per partition. Both defects have regression tests written to fail against the previous code. The demo profile is byte-identical: these columns sit under the sample bound, so both schemes measure every row, and the fix only moves what the old one got wrong. Signed-off-by: Albert Cui <albcui@nvidia.com>
…e rows that held text
`_detect_verifiability` scored each candidate column as `extractable_answer /
texts` -- the rows that yielded text, which is each column's own denominator. A
column present in one row out of a thousand therefore scored 1.0, and outranked
the genuine answer column at 0.8.
A profile of 1,000 rows whose `a` column carries `#### <n>` in 80% of them,
beside a `note` column null everywhere but the first row:
method: extractable_final_answer
coverage: 1.0
detail: "'note' ends with an extractable answer (#### or \boxed) in
100% of 1 sampled rows"
Not a rounding problem. The wrong column, asserted at full confidence -- and
`coverage` is the field the contract says a consumer may read literally once
`rows_complete` holds, so a verifier built from this profile would have been
pointed at `note`.
The denominator is now `probe.rows`. That is what the `ground_truth_column`
branch above it already divides by, and what the field is documented as: the
fraction of sampled rows carrying a usable verification target. Dense columns
are unaffected and every existing expectation holds unchanged, because the two
denominators only diverge on a column that is mostly absent.
Two regression tests, both written to fail against the previous code.
Signed-off-by: Albert Cui <albcui@nvidia.com>
…ition
The sampling cycle strides over a column's present strings (`_seen`), but it was
sized from `expected_rows`, which counts the partition's rows. On a column that
is mostly null those are different units, and a cycle sized in the wrong one
never completes a single revolution: only the first block is ever eligible and
the sample collapses onto the head of the column -- the precise bias blocks were
introduced to remove one commit ago.
Measured, the same data written twice:
parquet (footer -> expected_rows known): repetition_score = 0.000
jsonl (adaptive, no footer): repetition_score = 0.500
Truth is 0.500. The path with the declared schema -- the one the module
docstring calls sharper -- is the one that reported a half-degenerate column as
perfectly clean. It bites whenever a column's value count falls between the
block and the cycle: 2,000 values in a 200,000-row partition measured its first
512 and stopped.
The cycle now starts at one block and widens from the density actually observed,
which puts both counters back in the same units. Two things that turned up on
the way:
**Retargeting between batches is not enough.** `measure_columns` hands the whole
partition over as a single batch, so a width settled only at batch boundaries
would never move at all. Retargets also fire every block of present values.
**A density read mid-batch reads low.** The row counter is already at the end of
the batch while `_seen` is still walking through it, so a column with no nulls
at all reads 0.5. Letting that narrow the cycle made it collapse and re-widen
once per batch, and scan 97,385 values against a 50,000 budget. Mid-batch
retargets widen only; the call at the end of the batch, where both counters
describe the same rows, is the authoritative one.
`DeferredAccumulator` drives this accumulator's `_observe` directly and never
feeds the row counter the retarget reads, so it now hands the count over. The
inferred path places its sample in the same rows as the declared one.
Measured after, driven in pipeline-sized batches, truth 0.500 throughout:
dense 200k / 1M rows 0.504 / 0.505 50,176 values scanned
sparse 2,000 of 200k 0.500 2,000 values scanned
sparse 1,000 of 400k 0.500 1,000 values scanned
The dense path costs what it did before: 50,176 against a 50,000 budget.
Two regression tests. The first fails against the previous code; the second
guards the row hand-over, and fails when only that line is removed.
Signed-off-by: Albert Cui <albcui@nvidia.com>
Three things this branch was failing CI on, none of them behavioural. `plugins/nemo-datasets/pyproject.toml` shipped without the SPDX header every other plugin manifest carries, which `check-copyright-headers` rejects. `SamplingInfo(row_budget=1024)` survived in the contract test after 8dcda13 removed that field from the contract. Pydantic's `extra="ignore"` -- which the module docstring keeps deliberately, so an older consumer tolerates a newer profile -- swallowed it, and the test went on passing while claiming to exercise every model in the contract. `test_schema.py` indexed through `fields` and `items`, both `| None`, which `lint-python-types` reports as `not-iterable`. That is not one of the rule classes CI globally suppresses, so it failed the job. A `_fields()` helper asserts the node is the container the test already expects it to be, which states the same expectation and reports a wrong shape as the assertion it is rather than as an AttributeError from inside a comprehension. Signed-off-by: Albert Cui <albcui@nvidia.com>
5cf4a63 to
2bd642d
Compare
Removes `TextQuality` -- whitespace, non-ASCII and repetition ratios -- and the sampling machinery that existed to make them affordable. Net 576 lines out. They were the only measurement of their kind here. Every other statistic is exact and O(1) per row; these three were the only *estimates*, the only *sampled* values, and the only per-character work in the profiler, measured at roughly 37x the cost of every content probe combined. One feature carried all of the engine's statistical subtlety, and reviewing it meant reviewing a sampling scheme buried in a change that is mostly about something else. What goes with them is the point. The block stride, the per-block weighting, the adaptive doubling for a column of unknown length, and the density correction that keeps a stride over *present strings* in the same units as a row count -- including its widen-only guard, which exists because a density read mid-batch reads low on a column with no nulls at all. All of that was in service of these three numbers. `expected_rows` goes too. It was threaded from `_peek_files` through `_PartitionFolds`, `ColumnFold`, `InferredColumnFold`, `_accumulator_for` and `DeferredAccumulator`, and its sole consumer was sizing the quality cycle. Removing one field removes a whole plumbing dimension; `StringAccumulator` collapses from sixty-odd lines of stride bookkeeping to a length fold and a vocabulary. `Quantiles` stays and is still an estimate -- it reads percentiles off bucketed counters rather than off retained lengths. But that is one self-contained data structure whose error is a bound you can check by inspection, not a scheme with cross-batch state. `rows_complete` no longer names `TextQuality` as the second estimate, because there is only one now. Nothing consumes a profile yet, so removing a field costs nobody anything today, and the contract is built for it: the schema version is still 1.0 precisely because nothing reads it, adding a field back is a minor bump, and a test already pins that profiles written before a field existed keep loading. Deferring is cheap and, by design, reversible. The cost, worth stating plainly: until this lands again there is no corruption signal in a profile. The content probes look for answer markers and embedded transcripts, not for padding, encoding damage or degenerate loops. A consumer should not read the absence of a quality block as a clean bill of health. The implementation is in this branch's history to lift from when it returns, where the sampling scheme can be the subject of review rather than a detail inside 5,800 lines. Signed-off-by: Albert Cui <albcui@nvidia.com>
Format is decided by extension, so anything named .parquet reaches the parquet reader -- a truncated upload, an HTML error page saved under the requested filename, a file that was never parquet. All three came back as a decode failure from inside pyarrow, which says the profiler could not read the file but not that the file is the wrong shape. A misnamed .jsonl was worse: forty thousand lines of "Expecting value", which reads like corrupt data and sends the reader looking at the wrong thing. Parquet checks PAR1 at both ends. Keeping the two markers apart is what separates "this was never a parquet file" from "the bytes stop early", which point at different problems. The size guard is not redundant: four bytes of PAR1 pass both marker checks, since the leading and trailing reads land on the same bytes. Line-delimited JSON has no signature of its own, so its check can only say what the file is not, and it lists binary signatures only -- none of those bytes can begin a text file, so the check cannot have a false positive. A textual mismatch is deliberately left alone: a pretty-printed JSON array is still text, a first line that is not an object is not proof the rest are not, and this reader's stance on a bad line is that it costs that line rather than the file. Checked from every entry point rather than peek() alone. The pipeline peeks a whole partition before reading any of it and discards those failures, on the grounds that a file that cannot be peeked will fail again with a reason when it is read -- so the read path is where the message has to be raised for it to reach a FileError at all. The reader tests are parametrized across all three so a check guarding only one of them fails. One cost: jsonl peek() opened nothing before and now opens the file for sixteen bytes. Negligible locally; against a ranged source it is an extra block fetch per shard. Signed-off-by: Albert Cui <albcui@nvidia.com>
No source has ever populated it and nothing has ever read it. Both references were tests asserting it stays None, which is not a behaviour worth pinning -- it only restated that the field was dead. A field on a shape the profiler passes around is a promise that something will fill it in. Leaving it declared invites a consumer to branch on a value that is structurally always None, and invites a reviewer to wonder which source is supposed to set it. Neither is a question worth carrying. The Files listing does not carry a digest either, so the platform source would not have populated it. If integrity checking is wanted later it needs a producer first, and adding the field back alongside one is a smaller change than keeping it hollow until then. Signed-off-by: Albert Cui <albcui@nvidia.com>
The plugin shipped with no overview. The reasoning behind each decision lives in module docstrings, which is the right place to maintain it but the wrong place to meet it: a reader has to already know which module to open before any of it is reachable. Three mermaid diagrams -- the pipeline, the fold, and the measure stage -- plus the design decisions stated as decisions rather than as description: why reading everything is the default, why every file is opened, why quantiles instead of mean, why a histogram instead of a reservoir, why the profile almost never contains row content, and why an absent field is a claim rather than a gap. The worked examples are real output over real datasets -- gsm8k, no_robots, alpaca and ultrafeedback_binarized -- not fixtures shaped to flatter the engine. They earn their place by showing behaviour that only appears on data nobody constructed for it: gsm8k's two configs becoming two partitions with meaningfully different sequence budgets, no_robots forcing `data/train-*.parquet` because `train` sits beside `train_sft`, and ultrafeedback carrying rewards on a 1-10 scale rather than [0,1]. Two rough edges are documented rather than hidden, since a reader meets both immediately: `LocalFileSource` does not skip dotted directories, so a `snapshot_download` cache turns the row total unknown, and a split name keeps its shard marker when a content hash follows it. Signed-off-by: Albert Cui <albcui@nvidia.com>
…g it
The `{from, value}` spelling of a chat message was documented by pointing
at one dataset family that uses it. The behaviour has nothing to do with
that family: `_MESSAGE_KEY_SETS` accepts the shape, whoever wrote it, and
the comments read better stating what the profiler matches than naming an
example of it.
Every mention replaced rather than deleted, so the reasoning survives --
which spelling, why recognising only `{role, content}` was a bug, and what
it cost. Test names follow, since a name is documentation a reader trusts
the same way.
Fixes a duplicated clause in schema.py's module docstring while rewriting
the sentence it sat in: the `messages` dtype was introduced twice in a row.
The role vocabulary example in `MessageStats.roles_seen` is part of the
stored contract, so this changes a field description that the OpenAPI spec
and SDK carry. Neither is generated on this branch; the branch stacked on
it regenerates them.
Signed-off-by: Albert Cui <albcui@nvidia.com>
…olds
`ColumnFold` measured every column of a partition, but every other name in
the module using that prefix means exactly one thing -- `ColumnAccumulator`
and its five subclasses. It was the only place where a singular prefix
named a plural object, and it sat one line above the accumulator it holds.
`RowFold` names the input instead, which is the real distinction:
RowFold.update(rows: list[dict]) a batch of rows, fanned out
ColumnAccumulator.update(values: list[Any]) one column's values
That contrast is invisible when both are called `.update()` on something
prefixed `Column`. It also matches the vocabulary the rest of the profiler
already uses -- `row_budget`, `rows_scanned`, `rows_present`,
`rows_complete`, `MIN_ROWS_PER_FILE` -- where `record` or `table` would
have introduced a synonym for a thing already named.
`PartitionFold` would have been the more literal reading, and is why it was
not taken: `_PartitionFolds` already means "the two folds a partition
needs", and nesting one inside the other invites reading the outer as a
collection of the inner.
Pure rename. Ruff re-sorted the import and rejoined a ternary that now fits
one line; no other line changed.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`RowFold.finalize` returned measurements alone while `InferredRowFold.finalize`
returned `(features, measurements)`. The mismatch forced an `isinstance` branch
in `_PartitionFolds.measure` and made `self.features` a public attribute
reassigned inside `measure()` -- an out-parameter in disguise.
It also left `RowFold` unable to publish its own feature list, which is not the
one it was handed: the constructor drops duplicate field names, which parquet
permits. So the caller's raw copy reached the artifact and announced a column no
accumulator had ever measured:
declared [('q','string'), ('q','int64'), ('a','string')]
RowFold._features [('q','string'), ('a','string')]
features in artifact [('q','string'), ('q','int64'), ('a','string')]
stats keys ['a', 'q']
A consumer reading `features` saw two `q` columns; one reading `stats` saw one.
`_stats_keys_subset_of_features` does not catch it -- duplicates do not violate
a subset check.
Both folds now return the pair, `_PartitionFolds` takes `features` as a local
from whichever fold owns it, and the `isinstance` is gone. `measure_columns`
absorbs the shape change in one line, so no test call site moved.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`stats.py` is hard to read top-to-bottom: `RowFold` is the first class in the file and depends on almost everything below it. The README went straight to mermaid diagrams of a pipeline whose vocabulary -- fold, accumulator, deferred dtype -- it never introduced. Adds a plain-language section ahead of Architecture, built on one image that holds all the way through: a conveyor belt of boxes you may not keep, and a notepad whose size must not grow. Every constraint in the module follows from that, so the section derives them rather than listing them -- why quantiles are bucketed, why the vocabulary erases itself, why probes ride along on the same pass, why quoting runs after classification. Closes with the order to actually read the file in, which is not top to bottom. Numbers are measured, not recalled: the bucket-width table, the 1024/1025 saturation cliff, and the ~2.9x deferred cost were each produced by running the code. Names rather than line numbers throughout, since line numbers go stale and a README that lies is worse than one that is vague. Signed-off-by: Albert Cui <albcui@nvidia.com>
Two folds meant two mechanisms for one job. `RowFold` chose an accumulator per
column from a declared schema; `InferredRowFold` could not, because an inferred
dtype is a whole-column decision, so it carried a `DeferredAccumulator` that
measured every shape and picked at the end.
Converting jsonl to Arrow first and letting the parquet path handle both was the
obvious way to collapse them, and does not work. `pyarrow.json.open_json` does
stream -- peak grew only 106 -> 133 MB for a 3x larger file -- but it infers
from the first block and errors when a later one disagrees:
int then string at row 400 ArrowInvalid: changed from number to string
int then float ArrowInvalid: couldn't parse 1.5
new column appears late ArrowInvalid: unexpected field
struct field appears late ArrowInvalid: unexpected field
all-null then string ArrowInvalid: changed from null to string
The last three are an optional field, tool_calls on some messages, and a column
null in early rows -- not corrupt files. `read_json` handles four of the five
but materialises at ~2x file size (403 MB file -> 841 MB peak, against 73 MB for
the fold on 134 MB), and `Table.from_pylist` infers from the *first row only*,
dropping later keys silently. It is a modelling gap, not an API one: Arrow has
no honest type for a column holding two shapes, where we emit `json`.
So the deferred accumulator becomes the only one, and the fold that used to
choose now only says which measurement answers. `InferredRowFold` and
`_accumulator_for` are deleted. `features=None` means "discover the columns".
Two supporting changes. Measurements are built on first sight of a value that
needs one: eager construction cost every column all four, 7157 B empty of which
5109 B (71%) sat idle on a single-typed column. And the dtype dispatch, written
three times and required to agree, collapses to one five-line `_measurement_for`
-- naming the measurement rather than constructing it is what allows that.
Verified by profiling 24 fixtures before and after: 22 byte-identical. The two
that move are duplicate-field-name parquet, where `to_pylist` collapses the pair
to the last one's values while the schema reports the first one's type. Such a
column now reports the measurement for its declared dtype rather than the
cardinality of values of another type, which is the merge's only behavioural
change anywhere. Pinned by a test.
`test_one_unmeasurable_column_...` monkeypatched `_accumulator_for`; it now
patches `DeferredAccumulator._observe`, which states its intent without
depending on registry internals.
Signed-off-by: Albert Cui <albcui@nvidia.com>
A pass over the public types, taken now because `dataset_profile.py` is on main but imported by nothing except its own test -- absent from `openapi.yaml` and from the SDK. The surface is still unshipped, and stops being free to rename the moment the job PR wires it into the API. Three of fifteen were worth changing, and family consistency is what picked them out. Every suffix family in the module is coherent except in these places: `SamplingInfo` -> `Coverage`. Its own docstring already said "coverage, stated as numbers", and the surrounding prose had been calling it that for two commits. `row_budget` defaults to None now, so the common case is a complete read -- "Sampling" told a reader the profile rested on a sample when it usually did not. Sole `*Info` in the module; the suffix carried nothing. The artifact key moves with it, `profile["sampling"]` -> `profile["coverage"]`. `DeferredAccumulator` -> `RoutedAccumulator`. Its five siblings are named for what they measure; this one was named for when it decides, and since the fold merge nothing is deferred when the dtype is declared. What it does is route each value to the measurement that fits it. `ColumnMeasurements` -> `PartitionMeasurements`. It holds `dict[str, ColumnStats]` -- every column, not one -- while sitting two lines from `ColumnStats`, which is one. Its docstring already scoped it correctly. Left alone after consideration: `FeatureSchema` (75 refs for the least gain, and each node does carry a schema), `FilePreview` (pairs with `peek()`, so it is rename-both-or-neither), `Verifiability` (the current name is better -- `verifiability=None` reads as "not verifiable", which is the claim, where `verification_target=None` reads as the weaker "no target found"), and `RowFold` (`PartitionFold` was already weighed and rejected in 3b28a4e, and `Fold` alone is ambiguous among three fold classes). The field rename used anchored patterns rather than a word replace: `sampling` legitimately survives in prose about file-level sampling as a technique, and throughout other services where it means generation parameters. Signed-off-by: Albert Cui <albcui@nvidia.com>
The engine carried 31% prose. Much of it was a second and third paragraph of
rationale, history ("which previously cost the partition every measurement it
had"), and measured anecdotes ("2.6 MB beside 61.4 MB of resident rows") --
worth writing once, and now carried by the plugin README's "How the measurement
works" section, which ships in the repo beside the code.
The rule applied: what it is, plus at most one paragraph of why. Every invariant
stays, as does every warning a reader must not miss -- `max` is exact, absence
*is* the claim, `**` is never emitted.
stats.py 3136 -> 2145 words 32%
parquet.py 333 -> 232 30%
run.py 467 -> 336 28%
pipeline.py 1989 -> 1443 27%
splits.py 760 -> 558 27%
----------------------
total 9591 -> 7208 25%
Four comments were not merely verbose but wrong, left behind by the fold merge:
`pipeline.py`'s module docstring, its `_profile_partition` footer and branch
comments, and `stats.py`'s module docstring all still said that a declared schema
means the accumulators are "chosen up front". A refactor that renames a class is
caught by the compiler; one that invalidates prose is not, which is the argument
for reading every line rather than grepping for the names that moved.
Verified prose-only by dumping each file's AST with docstrings stripped, before
and after: identical. Tests pass and `ty` is unchanged at its 18 pre-existing
plugin diagnostics.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`Field(description=...)` values are published: they land in `openapi.yaml` and in
the generated SDK, so they are read by people who never open this file.
`SplitProfile.data_files` alone was 1243 characters, and `rows_complete` 763.
field descriptions 2046 -> 1428 words 30%
class docstrings 1730 -> 1242 28%
----------------------
total 3776 -> 2670 29%
Cut: the history behind each decision, and the alternatives weighed on the way --
why `exhaustive` was dropped from Coverage, why `CategoricalStats` stopped being a
general cardinality count, why the contract lives in a shared package. Those are
reasons to have written the field, not things a consumer needs at the point of
reading it.
Kept: what the field means, what its absence claims, and the one guarantee a
consumer acts on -- that `max` is exact, that a glob is never approximate, that
`rows_present` goes unknown rather than low.
No field is added, removed, renamed or retyped; this is description text only. It
does change the OpenAPI spec, so the SDK regenerates with it.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`_blocks` reads as a verb -- "it blocks" -- where every other accessor in the
module is a noun returning the thing it names: `vocabulary()`, `feature()`,
`quantiles()`, `values()`, with `_observe()` as the verb partner.
"Block" itself is the right word and stays, because it is the contract's own:
`ColumnStats`'s docstring already says "the kind-specific block is populated by
dtype", and the field set it fills is exactly {text, numeric, messages,
categorical}. Qualifying the plural is all that was needed.
def _observe(self, present: list[Any]) -> None: ...
def _stat_blocks(self) -> dict[str, Any]: ...
Nine references, all inside `stats.py`: the base declaration, five overrides, the
`finalize` call site, the delegation in `RoutedAccumulator`, and the docstring
naming the two hooks. Nothing outside the accumulator hierarchy touches it.
`_measured` was the runner-up and was dropped because `RoutedAccumulator`
already has `_measurements` for the sub-accumulators, and two near-identical
names for different things is worse than the ambiguity being fixed.
Signed-off-by: Albert Cui <albcui@nvidia.com>
The measurement section explained itself through a conveyor belt, boxes and a
notepad. The intuition was right and the register was wrong: it dumbed down
material aimed at someone about to read `stats.py`. Rewritten in plain technical
prose, keeping every table, number, code example and cross-reference, and losing
only the metaphor -- headings included, so "Which notepad answers" is now
"Choosing which measurement reports".
Two things got sharper in the process and are worth calling out:
- the constraint section now states the invariant outright -- splitting a column
across many `update` calls gives the same answer as one call with all of it --
where the metaphor had only gestured at it, and that equivalence is the thing a
reviewer should actually check;
- the histogram section now says the bucket edges are fixed in advance and never
adapted to the data, which is what makes two runs over the same bytes agree.
Adds "When a column's types disagree", which was the first question the section
left unanswered. Ints and floats widen; anything else resolves to `json`, which
is a claim rather than a fallback. `null_rate` and the content probes survive it,
since neither lives in a dtype block. The case worth watching is a role-named
column that disagrees, because `json` fails the role dtype gates:
completion = "a0", "a1", "a2", "a3" -> string completion -> prompt_completion
completion = "a0", "a1", "a2", 42 -> json None -> prompt_only
One malformed row in four. The profile is honest about it and nothing flags it,
so Limitations gains the gap that section links to: the profile describes, and
does not warn. Every row of the new table was verified against the code.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`RoutedAccumulator._observe` sorted every batch by python type before handing it
on. That is what an inferred column needs, because the dtype is not known until
the last row. A declared column already knows it, so three of the four scans
could only ever come back empty:
1024 values x 200 batches, declared string column
before 62.2 ms +46% over StringAccumulator alone
after 43.8 ms +1% (43.2 ms is the floor)
The `SchemaFold` was already skipped on this path; the routing was not.
The speedup is the smaller half. Handing the batch whole to the measurement the
declared dtype names is exactly what `RowFold` did before the two folds were
merged, so this restores the one behaviour that merge changed. A column whose
declared dtype disagrees with its values -- reachable through parquet duplicate
field names, where `to_pylist` collapses the pair to the last one's values while
the schema reports the first one's type -- reports its cardinality again:
pre-merge distinct_count=2
post-merge, pre-short-circuit distinct_count=0
now distinct_count=2
So the merge is now behaviour-preserving outright, rather than on 22 of 24
fixtures. That deviation was never a decision; it was a side effect of routing on
a path that had nothing to route.
`test_a_declared_dtype_is_answered_only_by_its_own_measurement` pinned the
deviation and is rewritten as `..._measures_every_value_it_was_given`, asserting
the restored behaviour instead.
Signed-off-by: Albert Cui <albcui@nvidia.com>
…easured `PartitionClassification.candidates` is documented as "most specific first, so `candidates[0]` is `dataset_type`", and `classify` upholds that by deriving `dataset_type` from the head of the list it just built. The wide guard around the measure stage is the one path that never reaches `classify`. It emitted `dataset_type="unknown"` and let `candidates` default to `[]`, so a consumer reading the documented primary candidate got an IndexError on exactly the profiles that were already degraded. It now supplies the head itself. Signed-off-by: Albert Cui <albcui@nvidia.com>
`_dtype_allows` returns True for any role it does not recognize, because
`id` / `provenance` / `meta` / `tools` / `image` carry no dtype constraint. A
typo'd role name took that same branch, so `{"q": "prmpt"}` was accepted:
semantic_role='prmpt' semantic_role_source='declared'
[column_name] columns matched roles: q -> prmpt
dataset_type='unknown'
Three things wrong at once. The profile stored an out-of-vocabulary
`semantic_role`, where the contract says vocabularies are open for *readers* but
only known values are emitted. The evidence affirmatively reported a match that
never happened. And nothing downstream reads `prmpt`, so the dataset classified
as `unknown` while the profile claimed a role had been assigned.
That is the failure mode `_dtype_allows` exists to prevent -- its docstring
already argues that a hint says which column, not what the data is -- caught for
a mistyped column name but not for a mistyped role name.
`_KNOWN_ROLES` is derived from the alias table rather than written out, so the
two cannot drift, and a hint outside it is reported as `user_hint` evidence
exactly as a dtype mismatch already was.
Signed-off-by: Albert Cui <albcui@nvidia.com>
`Path.is_file()` resolves a symlink and `open` follows it, so a link planted
inside a `LocalFileSource` root was listed and read like any other file. Its
column names then reached the profile:
list_files sees: ['escape.jsonl', 'train.jsonl']
columns profiled: ['leaked', 'a']
Not an active path today -- the root is a developer's directory or a test
fixture, the job reads a fileset through ranged requests rather than staging it,
and nothing extracts an archive that could plant a link. This is the cheap guard
to have in place before any of that changes.
A symlinked *directory* was already excluded, since `rglob` declines to descend
into one; this covers the file case. Resolved-root containment and a no-follow
open would additionally close the race where the tree changes under the profiler,
which is more than a local directory scan warrants.
Signed-off-by: Albert Cui <albcui@nvidia.com>
…t()-able
`int(requested)` accepted four shapes the step config has no business carrying,
and silently changed what three of them meant:
1.9 -> 1 profiles one row
true -> 1 profiles one row
false -> 0 which this function reads as "every row"
"5" -> 5
`false` is the worst of them: a caller writing it means "no budget" in the sense
of "off", and got an unbounded read. A budget that quietly means something other
than what the config says is worse than a job that refuses to start, and this
reads a file off disk, so nothing upstream is guaranteed to have checked it.
Now only a JSON integer is accepted -- bool excluded, since it is an int in
python -- and `0` and `null` keep their documented meaning of "every row".
Signed-off-by: Albert Cui <albcui@nvidia.com>
Dataset Profiler Engine
Adds a dataset profiler: given a fileset's files it derives the row schema, measures per-column stats, classifies what the data is, and emits a typed
DatasetProfile. Nothing consumes this contract yet, the Files service integration follows in a stacked PR.Basic usage
Given a local directory containing two datasets (partitions):
Run
{ "profile_schema_version": "1.0", "created_at": "2026-08-07T14:25:08.524649Z", "profiler_info": { "name": "nemo-dataset-profiler", "version": "0.1.0" }, "sampling": { "rows_scanned": 13392, // 10k budget per partition, split across its files "rows_present": 67551, // exact, from parquet footers — read or not "files_read": 4, "files_present": 4, "bytes_present": 46514872, "row_budget": 10000 }, "partitions": [ { "name": "helpsteer2", "file_formats": ["parquet"], "splits": [ { "name": "train", "canonical": "train", "num_files": 1, "size_bytes": 18495985, "num_examples": 20324, "data_files": "helpsteer2/train*.parquet"}, { "name": "validation", "canonical": "validation", "num_files": 1, "size_bytes": 963692, "num_examples": 1038, "data_files": "helpsteer2/validation*.parquet"} ], "features": [ { "name": "prompt", "dtype": "string", "semantic_role": "prompt", "semantic_role_source": "detected" }, { "name": "response", "dtype": "string", "semantic_role": "completion", "semantic_role_source": "detected" }, { "name": "helpfulness", "dtype": "int64", "semantic_role": "score", "semantic_role_source": "detected" }, ... // correctness, coherence, complexity, verbosity — all score ], "stats": { "prompt": { "null_rate": 0.0, "text": { "chars": { "p50": 265, "p95": 2692, "p99": 3089, "max": 3950 } }, "categorical": { "distinct_count": 3019 }, "quality": { "whitespace_ratio": 0.1699, "non_ascii_ratio": 0.0025, "repetition_score": 0.0009 } }, "helpfulness": { "numeric": { "min": 0.0, "max": 4.0, "mean": 2.8538 }, "categorical": { "distinct_count": 5 } }, ... }, "stats_complete": false, "classification": { "modality": "text", "dataset_type": "scored_response", "candidates": ["scored_response", "prompt_completion"], "format": "standard", "prompt_form": "explicit", "evidence": [ { "kind": "column_name", "detail": "columns matched roles: prompt -> prompt, response -> completion, helpfulness -> score, ..." }, { "kind": "column_dtype", "detail": "standard format from role column dtypes" } ] } }, { "name": "hh-rlhf-helpful-base", "splits": [ { "name": "test", ... }, { "name": "train", ... } ], "features": [ { "name": "prompt", "dtype": "messages", "semantic_role": "prompt", ... }, { "name": "chosen", "dtype": "messages", "semantic_role": "chosen", ... }, { "name": "rejected", "dtype": "messages", "semantic_role": "rejected", ... } ], "stats": { "prompt": { "messages": { "turns": { "p50": 3, "p95": 9, "p99": 13, "max": 61 }, "content_chars": { "p50": 342, "p95": 1463, "p99": 2230, "max": 3923 }, "roles_seen": ["user", "assistant"], "ends_with_assistant_rate": 0.0, // ends on a *user* turn: it is a prompt "valid_alternation_rate": 0.9983 } }, "chosen": { "messages": { ..., "ends_with_assistant_rate": 1.0 } }, ... }, "classification": { "dataset_type": "preference_pair", "candidates": ["preference_pair"], "format": "conversational", "prompt_form": "explicit", ... } } ], "file_errors": [] }Some design decisions worth calling out
1. The
DatasetProfileschema lives atpackages/nemo_platform_plugin/src/nemo_platform_plugin/files/dataset_profile.py, while the profiler itself lives in a separateplugins/nemo-datasets.If we put the contract in the dataset plugin, then a core service depends on an optional plugin, which goes against the architecture we are aiming for. It makes more sense to define the profile schema in the Files package because it is the first-class consumer of this schema, and will store it once we integrate with FileSets.
2. Partitions vs splits
A partition is the top-level container of a set of files with shared schema, and it can be further broken down into splits (e.g.
train,val,testetc). Partition names are based on top-level directory names, except when there are no top-level directories, or when the top-level directories happen to be split names liketrainorval, in which case a empty-string""is used for the partition name (the root partition), and split accordingly.In the future, we will add the capability to leverage user provided split configurations (either via the
profileentrypoint, or we can parse the optionalREADME.mdfront-matter in HF dataset repos).3. Splits and glob patterns
Given files
We resolve two splits, and each split gets a glob pattern to represent the files
Checkout
infer_data_files(splits.py) for how we implement this.4. Batching, folds, accumulators, and stats
In addition to understanding the schema of a dataset, we also extract stats from individual columns. For example, the length of the string sequences in the dataset is useful for coming up with a meaningful training budget (
max_seq_length). However, this means we need some way of measuring sequence lengths across all rows, and do some bookkeeping to keep track of the p50, p95, p99s etc. We also need to do this in a scalable way for large datasets. The solution is to accumulate batches of data in a loop, "fold" them in (counting, min, max etc), then throw away the batch so we never accumulate more than one batch worth in memory.At a high-level the loop looks like:
The unit of measurement is type specific. For example, the
MessageAccumulatorcontains histograms for sequence lengths, which gets bucketed (_length_bucket()), and counted. Finally, the quantiles are computed from the bucket counts.5. JSONL vs Parquet, and deferred dtype inference
In order to accumulate a specific column's values, we need to dispatch to the an accumulator that is aware of the dtype of the value, so that a number gets accumulated by
NumericAccumulator, while a string gets accumulated byStringAccumulator.However, with JSONL, we actually don't know the dtype of each column upfront, without actually loading the data. To solve this, we have
RoutedAccumulator, which is a higher level abstraction on top of the type specific accumulators.RoutedAccumulatorresolves the dtype usingisinstance(value, T), and dispatches to the type specific accumulator accordingly.The same isn't true for Parquet files, where the dtype is known in the footer. In this case, build the type-specific accumulator, and short-circuit. However, both go through
RoutedAccumulator, so we have a single entrypoint.6. Removed content-digest. There was a
content_digestfield, which was originally meant as a field for change detection, where a subsequent job can try to compute a content_digest and see if it conflicts with a previous one's. However, it gets complicated when the content_digest might not be based on all the files in the dataset (we only profile the files we care about like jsonl and parquet). I don't want to deal with this complexity right now, and I don't think it should be the dataset profiler's responsibility to compute the content digest.Summary by CodeRabbit
New Features
profile.jsonartifacts.Bug Fixes