Repository navigation
feat: add dtype_backend to result loading - #1128
binaryaaron wants to merge 4 commits into
Conversation
DatasetCreationResults.load_dataset() always read parquet with
dtype_backend="pyarrow", so nested columns came back as ArrowDtype.
Add a keyword-only dtype_backend ("pyarrow" default, "numpy_nullable",
or None) threaded through ArtifactStorage.load_dataset() and
read_parquet_dataset(), including the schema-unification and encoded
JSON storage branches. None omits the pandas option, so pandas applies
its default conversion (nested columns as object).
The default is unchanged. CompositeWorkflow.load_dataset does not yet
expose the option.
🤖 Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Document that dtype_backend=None follows pandas conversion and stored metadata. Encoded JSON columns retain object dtype for every backend; loading does not repair incompatible persisted Arrow dtype metadata. Extend the public result-loading regression through a Parquet write and pandas default read for None and numpy_nullable. Preserve the existing pyarrow default. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Use an explicit typed nullable dtype mapping in encoded storage. Consolidate public coverage around real result loading for each backend and remove redundant pandas export/read-back coverage. Preserve the Arrow default and decoded JSON values. Export compatibility and nested-container normalization remain outside the loading contract. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Exercise native and JSON-encoded backend selection through real result storage, and reuse the numeric-promotion regression across backends. Keep focused fallback and invalid-input checks while removing duplicate storage/reader tests and their setup helpers. Compare dtype objects directly so the tests express conversion semantics across supported pandas versions. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
|
Fern preview: https://nvidia-preview-pr-1128.docs.buildwithfern.com/nemo/datadesigner
|
[Medium impact] The PR appears safe to merge; no actionable defects were found. SummaryAdds
binaryaaron explicitly noted that persisted pandas metadata still applies and nested-container normalization is unchanged.
|
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
No qualifying findings.
I reviewed all seven changed files against the base and traced the result-loading path through engine storage and both native and JSON-encoded readers. The public API preserves the Arrow default. Explicit backend choices reach file, directory, and schema-unification fallback reads, while decoded JSON columns retain object dtype.
The inspected regressions cover public loading through real storage, native nested numeric promotion, fallback conversion, and file/directory consistency. I couldn't execute tests in this read-only environment; this approval is based on source and test inspection.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
|
Thanks for putting this together, @binaryaaron! SummaryPR #1128 adds dtype_backend selection to DatasetCreationResults.load_dataset and forwards it through artifact loading while preserving the Arrow default. Two P2 issues affect native-column dtypes when loading JSON-encoded artifacts. Could you share the concrete downstream use case driving this API addition? Which consumer needs FindingsWarnings — Worth addressing
What Looks Good
Residual RiskValidation: 271 targeted tests passed on pandas 3.0.5 / PyArrow 24.0.0, and lint/format checks passed for all seven changed files. The string-inference and categorical failures were reproduced through DatasetCreationResults.load_dataset; the fixed-offset timestamp concatenation failure was also reproduced. The full suite and pandas 2.3.3 matrix reported by the author were not independently rerun. VerdictNeeds changes — align pandas conversion semantics and preserve categorical dtype across encoded batches. This review was generated by an AI assistant. |
@nabinchha, I recommend fixing the reader here and reviewing the Arrow update independently. Both findings reproduce. The timezone mismatch specifically reproduces with pandas 3 / Arrow 24 when The tested options are:
Proposed reader fix: validate each fragment's JSON contract, read native columns together through public The shorter alternatives failed concrete cases: reading everything through pandas before JSON decoding rounded a native nested integer from The prototype changes three files (+91/−61 relative to this head), with 9 net production lines added and no new test functions. Existing public-load cases gain two batches with differing categories, fixed-offset timestamps, and disabled string inference, comparing native columns against pandas. These expose failures on this head and pass in all six combinations of pandas 2.3.3/3.0.5 × Arrow 24.0.0/25.0.0/26.0.1. The broader matrix passed 468 checks per combination, including 60 exploratory probes kept outside the proposed patch. Independent dependency proposal: widen all three constraints to On motivation: this originated in Anonymizer's result/export integration, but this API does not fix its nested-Arrow metadata export failure. The intended benefit is choosing pandas' read semantics at the artifact boundary instead of manually reading files or casting afterward. A separate consumer requiring |
📋 Summary
Let callers choose pandas dtypes through
DatasetCreationResults.load_dataset(dtype_backend=...), avoiding direct artifact reads or post-load casts. Preserve the Arrow default and decoded JSON values.🔗 Related Issue
N/A — no matching dtype-selection issue found. Upstream plan review is pending.
🔄 Changes
"pyarrow"as the default; accept"numpy_nullable"andNonefor pandas-default conversion.🧪 Testing
make test: 5,019 passed, 3 skipped.make check-all, commit hooks, and diff checks.✅ Checklist
Prepared with AI assistance.