[REVIEW] Feat/interleaved io readers writers - #1570
Conversation
… writer - InterleavedParquetReaderStage: native pyarrow reading with null-fill for missing columns and large_string/large_binary compat in reconcile_schema - InterleavedParquetReader: composite stage (FilePartitioning + reader) - InterleavedWebdatasetWriterStage: reconstructs WDS tar shards from interleaved rows with fsspec cloud path support - InterleavedLanceFragmentWriterStage: writes lance fragments via write_fragments() API; commit_lance_fragments() assembles into dataset - BaseInterleavedWriter: add on_materialize_error parameter, fix storage_options leaking into write_kwargs - Promote reconcile_schema from WebdatasetReaderStage to BaseInterleavedReader - Pin fsspec>=2025.2.0 and s3fs>=2025.2.0 - Extend benchmark script with --reader-type, --writer-format, --no-filter - Add WDS/Lance output metrics collectors - 13 new tests (148 total pass) Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
- Add lance_schema parameter to InterleavedLanceFragmentWriterStage: when set, every fragment is padded/reordered to match the declared schema (missing columns become null arrays). This prevents lance's parallel scanner crash caused by heterogeneous schemas across fragments. - Add _align_table_to_schema() helper for null-filling missing columns - Add _enforce_schema() to BaseInterleavedWriter: casts core columns to INTERLEAVED_SCHEMA types via reconcile_schema before every write - Apply _enforce_schema in WDS and lance writer write_data overrides - Use pa.unify_schemas in commit_lance_fragments for union schema - Update benchmark results markdown Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
Move all scattered schema logic into a single shared module: - reconcile_schema: canonical type resolution (from BaseInterleavedReader) - align_table: null-fill + reorder + cast (from lance _align_table_to_schema) - serialize_schema / deserialize_schema: IPC encoding (from lance.py) Add output_schema parameter to both BaseInterleavedReader and BaseInterleavedWriter with _align_output() method that applies align_table when output_schema is set, or reconcile_schema + cast when not. All readers (parquet, webdataset) and writers (parquet, webdataset, lance) now use the same centralized functions through their base class. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Rewrite _fill_range_read_rows to use a single shared filesystem object and a single cat_ranges call across ALL paths, instead of looping per-path with separate connections. This reuses the aiobotocore connection pool and amortizes TLS handshake costs. Before: 126-147ms/image (per-path url_to_fs + cat_ranges loop) After: 18-37ms/image (single batched cat_ranges, global dedup) Also add --source-ref-filter flag to benchmark script (default=s3) to drop samples with non-S3 source_refs before materialization. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
- reconcile_schema: decode dictionary-typed passthrough columns to their value type (fixes crash on merged parquet with domain_bucket column); also restore missing continue statement after dictionary branch - WDS writer: always record image name in JSON even when binary_content is null (e.g. no materialization). This preserves the interleaving structure on round-trip. Binary tar members are still only written when actual bytes exist. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
…andling The WebDataset writer was silently dropping per-image and per-text metadata columns (e.g. image_metadata) when writing tar shards. This commit: - Collects per-image/text extra column values from image/text rows and rebuilds them as parallel lists in the JSON payload, matching the original WDS convention. - Parses JSON-encoded dict/list strings back to native objects so sample- level fields like language_id_whole_page_fasttext round-trip correctly. - Filters None results in BaseStageAdapter.process_batch() to prevent AttributeError when a stage filters out all tasks in a batch. - Adds tests for per-image, per-text, and sample-level metadata round-trip. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
Rewrite _write_tar to extract all columns to Python lists once via pyarrow to_pylist(), then use O(1) list indexing per row instead of df.iloc[indices] + iterrows(). On ArrowDtype DataFrames, iloc is ~234ms/sample; list indexing is 0.054ms/sample (4,315x speedup). Also fix: dictionary columns decoded in reconcile_schema (missing continue after passthrough branch), and WDS writer always records image names in JSON even without binary content. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com> Made-with: Cursor
VibhuJawa
left a comment
There was a problem hiding this comment.
Added some notes for me to fix/review. Mostly code clean up things
There was a problem hiding this comment.
Pull request overview
Adds interleaved IO functionality (native Parquet reader, WebDataset writer, Lance fragment writer) and centralizes Arrow schema alignment so interleaved readers/writers can emit consistent schemas across backends and file formats.
Changes:
- Introduces
InterleavedParquetReaderStage,InterleavedWebdatasetWriterStage, andInterleavedLanceFragmentWriterStage(+ fragment commit helper). - Centralizes schema reconciliation/alignment and schema (de)serialization in
nemo_curator/stages/interleaved/utils/schema.py, and wiresoutput_schemainto base interleaved readers/writers. - Updates materialization range-read batching and extends benchmarking + dependency pins + tests for the new stages.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Adds/pins dependencies needed for multimodal/interleaved IO (notably fsspec, s3fs, pylance + lance namespace deps). |
pyproject.toml |
Pins fsspec/s3fs, adds multimodal_cpu extra, updates test deps. |
nemo_curator/tasks/interleaved.py |
Adds Lance blob-encoding metadata to binary_content field. |
nemo_curator/stages/interleaved/utils/schema.py |
New centralized schema utilities: reconcile/align + IPC hex (de)serialization. |
nemo_curator/stages/interleaved/io/readers/base.py |
Adds output_schema + _align_output() for Arrow table alignment in readers. |
nemo_curator/stages/interleaved/io/readers/parquet.py |
New native-pyarrow interleaved Parquet reader stage with null-fill for missing requested columns. |
nemo_curator/stages/interleaved/io/readers/webdataset.py |
Makes source_id_field optional and switches to base _align_output(). |
nemo_curator/stages/interleaved/io/reader.py |
Adds composite InterleavedParquetReader; makes WDS source_id_field optional. |
nemo_curator/stages/interleaved/io/writers/base.py |
Adds schema alignment in writers + configurable materialization error policy; avoids leaking storage_options into writer kwargs. |
nemo_curator/stages/interleaved/io/writers/webdataset.py |
New WebDataset writer that reconstructs tar shards from interleaved rows using fsspec. |
nemo_curator/stages/interleaved/io/writers/lance.py |
New Lance fragment writer + commit_lance_fragments() helper for distributed writes. |
nemo_curator/stages/interleaved/utils/materialization.py |
Refactors range reads to attempt a global deduped cat_ranges call. |
nemo_curator/stages/interleaved/utils/validation_utils.py / tests/stages/interleaved/test_validation_utils.py |
Removes require_source_id_field validation and its tests. |
nemo_curator/stages/interleaved/utils/__init__.py |
Re-exports new schema utilities; removes require_source_id_field. |
nemo_curator/stages/interleaved/io/{__init__.py,readers/__init__.py,writers/__init__.py} |
Exposes new reader/writer stages and commit helper. |
nemo_curator/backends/base.py |
Filters out None tasks from backend stage results. |
benchmarking/scripts/utils.py |
Adds output metrics collection for WebDataset and Lance outputs. |
benchmarking/scripts/multimodal_mint1t_benchmark.py |
Extends benchmark to support multiple readers/writers, optional filtering, and Lance commit. |
benchmark_results/INTERLEAVED_IO_BENCHMARK.md |
Adds benchmark results documentation for interleaved IO formats. |
tests/stages/interleaved/test_webdataset_writer.py |
New unit/integration tests for WebDataset writer behavior and round-trip. |
tests/stages/interleaved/test_lance_writer.py |
New unit tests for Lance fragment writing and commit. |
tests/stages/interleaved/test_interleaved_parquet_reader.py |
New tests for Parquet reader behavior and schema reconciliation. |
tests/stages/interleaved/test_multimodal_reader.py / test_multimodal_writer.py |
Updates tests for optional source_id_field and new writer error policy parameter. |
… tests Bug fix: _extract_tiff_frame reused the source JPEG compression when saving an extracted frame. JPEG does not support alpha channels, so RGBA frames (e.g. MINT-1T PDFs) produced corrupt pixel values (MAE ~46) and a broken alpha channel. Fix: copy the frame to force a full pixel decode, then save without specifying compression (lossless default). Test improvements: - Add build_jpeg_in_tiff fixture to conftest for regression coverage - Add test_extract_tiff_frame_jpeg_in_tiff_preserves_pixels regression test - Parametrize test groups across test_materialization, test_multimodal_core, test_multimodal_reader, and test_webdataset_writer to reduce duplication - Add test_explicit_schema_aligns_table covering schema= and schema_overrides= paths in InterleavedParquetReaderStage Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…lds, benchmarking - Add schema_overrides parameter to readers/writers (merge with INTERLEAVED_SCHEMA) - Support passthrough source fields in parquet/webdataset writers - Fix webdataset writer to preserve extra columns through round-trips - Expand benchmarking scripts with output metrics and verification modes - Fix tests/conftest.py: use /tmp for Ray temp dir to avoid >107-byte Unix socket paths - Fix test_lance_writer.py import ordering and noqa annotations Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Previously the policy was only applied when materialize_on_write=True and a fetch was triggered. Errors injected by upstream stages (e.g. ImageValidationStage setting materialize_error on corrupt images) were silently ignored, causing corrupt-image rows to pass through instead of being dropped. Restructure so the drop/fill policy runs unconditionally after the optional fetch step, regardless of how materialize_error was set. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…ove API - Remove InterleavedLanceFragmentWriterStage and test_lance_writer.py (redesign as CompositeStage with LanceCommitStage barrier in follow-up PR) - Remove benchmark_results/INTERLEAVED_IO_BENCHMARK.md; add benchmark_results/ to .gitignore - Fix _escape_key: encode % as %25 first so encoding is injective (B4) - Fix align_table: use safe=True cast for passthrough columns to prevent large-string overflow corruption; reserved columns keep existing logic (B5) - Fix materialization: group paths by filesystem in _build_global_range_index so mixed-backend batches (e.g. S3 + local) work correctly (B3) - Fix parquet reader: guard pq.read_schema() call behind `if self.fields is not None` to avoid N+1 I/O in the default read-all-columns case (C1) - Fix _resolve_schema return type annotation: pa.Schema | None (C4) - Fix _resolve_schema: warn when schema_overrides ignored because schema= set (C5) - Fix _resolve_schema: preserve orig.nullable from INTERLEAVED_SCHEMA (C6) - Fix webdataset writer: raise ValueError on unsupported modalities (previously silently skipped rows), remove dead _write_sample, make _write_dataframe non-abstract in base, remove local pyarrow import, rename tiff result to tiff_frame (D2, D3, D4, D5) - Remove lance imports from __init__.py exports and benchmarking script - Add tests: decompose() for InterleavedParquetReader, _escape_key collision proof, align_table safe-cast, materialization multi-filesystem grouping - Update quickstart notebook and benchmarking script to remove lance references Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…e-existing test files) Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…alse positives Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…ead (no TIFF frame extraction) Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
The RAY_ADDRESS pinning fix (import os + gcs_address pinning + restore in finally block) was accidentally bundled into the interleaved IO PR. Restore nemo_curator/backends/xenna/executor.py to its main branch state. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
| # Use /tmp to keep the path short — Ray Unix socket paths must be ≤107 bytes, | ||
| # and pytest's default basetemp (e.g. /raid/…/pytest-12/ray0) can exceed that. | ||
| temp_dir = tempfile.mkdtemp(prefix="nc_ray_", dir="/tmp") |
There was a problem hiding this comment.
Any opinion on this ? I was running into issues here of pytesting , without this ?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 29 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/conftest.py:207
shared_ray_clusternow creates a Ray temp dir viatempfile.mkdtemp(...)but never deletes it. Since this fixture issession-scoped andautouse, it can leak/tmp/nc_ray_*directories across test runs. Consider usingtempfile.TemporaryDirectory(...)(and yielding its path) or cleaning up withshutil.rmtree(temp_dir, ignore_errors=True)in thefinallyblock afterray_client.stop().
# Use /tmp to keep the path short — Ray Unix socket paths must be ≤107 bytes,
# and pytest's default basetemp (e.g. /raid/…/pytest-12/ray0) can exceed that.
temp_dir = tempfile.mkdtemp(prefix="nc_ray_", dir="/tmp")
ray_client = RayClient(
num_cpus=num_cpus,
num_gpus=num_gpus,
object_store_memory=object_store_memory,
ray_temp_dir=str(temp_dir),
include_dashboard=False,
The tempfile.mkdtemp(dir='/tmp') change was motivated by a Ray Unix socket path length issue on the local /raid/... dev path, but: - Interleaved tests (the scope of this PR) don't use shared_ray_cluster - tempfile.mkdtemp doesn't auto-clean like tmp_path_factory, leaving stale dirs on CI runners Restore tests/conftest.py to upstream/main state. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Restore the upstream/main merge logic that reads existing params.json / metrics.json and updates values rather than overwriting the whole file. The simplified overwrite was an unintended scope creep in this PR. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…type - BaseInterleavedWriter._write_dataframe: raise NotImplementedError instead of silently succeeding; subclasses that bypass write_data() would otherwise produce empty output files with no error (Copilot #2) - InterleavedParquetReader.schema_overrides: tighten annotation from dict[str, Any] to dict[str, pa.DataType] to match _resolve_schema expectations (Copilot #4) Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
- Remove _try_read_lance_dataset and collect_lance_output_metrics from benchmarking/scripts/utils.py — lance was removed from this PR and these functions were unused (not imported by any benchmark script) - Remove 'Lance writer' example from BaseInterleavedWriter docstring Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Consistent with InterleavedParquetReaderStage and InterleavedWebdatasetWriterStage naming convention. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…ebdataset writer Add targeted tests for previously uncovered lines in: - utils/schema.py: reconcile_schema (large_binary compat, dict passthrough unwrapping, nullable=False preservation) and align_table (drop/reorder columns, reserved binary reconciliation, safe upcast) - io/writers/base.py: all on_materialize_error modes (error/warn/drop_row/ drop_sample), _write_dataframe NotImplementedError, _align_output with and without schema - io/writers/webdataset.py: _escape_key edge cases, _ext_from_content_type known and unknown MIMEs, _build_index insertion order, unsupported modality raises ValueError, empty batch writes empty tar - io/readers/base.py: _resolve_schema (schema wins over overrides, both-None raises, overrides preserve nullable) Also document the RAY_ADDRESS workaround for shared-machine test runs in CLAUDE.md. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
…le varieties Adds test_wds_library_loads_json_and_image_in_same_sample covering: - text-only samples (no binary members) - multi-image samples (JPEG + PNG, bytes and extension verified) - interleaved text+image samples (mixed ordering) - percent-encoded sample IDs (special chars in key) Also fixes _escape_key docstring: WDS groups by prefix before first '.' not by Path.stem as previously stated. Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
Signed-off-by: Vibhu Jawa <vjawa@nvidia.com>
|
/claude review |
abhinavg4
left a comment
There was a problem hiding this comment.
This is too big to review. Can you please split into smaller PRs, maybe one for WDs and another for parquet?
Summary
Adds two new interleaved IO stages and centralizes schema alignment for all arrow-based readers/writers:
schema/schema_overridesfor schema alignment.fsspec.open()for cloud path support. Groups bysample_id, writes JSON metadata + binary image members per sample. Keys are percent-encoded (injective —%escaped first as%25) to prevent collisions.Centralized schema alignment (utils/schema.py)
All schema logic moved to a single module shared by all readers and writers:
reconcile_schema(inferred)align_table(table, target)safe=Falseonly for reserved large↔small downcasts;safe=Truefor passthrough columns to surface overflow errorsBoth
BaseInterleavedReaderandBaseInterleavedWritergainschema/schema_overridesparameters and_align_output().Other changes
on_materialize_errorparameter (error/warn/drop_row/drop_sample); policy now applies to both fetch errors and errors set by upstream stages (e.g.ImageValidationStage)._write_dataframechanged from@abstractmethodto a concreteNotImplementedError-raising default so subclasses overridingwrite_data()directly don't need to implement it, while still failing fast if neither is overridden.InterleavedParquetReader.schema_overrides: tightened annotation fromdict[str, Any]todict[str, pa.DataType]to match_resolve_schemaexpectations._escape_keyinjectivity fix + docstring: Percent-encoding now encodes%first (as%25), making the mapping injective —"a.b"and"a%2Eb"always produce distinct tar keys. Docstring corrected: WDS groups by prefix before the first".", not byPath.stem._build_global_range_indexreturns one(fs, unique_ranges)pair per distinct filesystem backend, enablingcat_ranges()to be dispatched correctly when a batch spans multiple storage backends (e.g. local + S3)._resolve_schema: RaisesValueErrorwhen bothschema=andschema_overrides=areNone. Logs a warning if both are supplied simultaneously (only one is applied).pq.read_schema()is no longer called whenfields=None(read all columns), avoiding an unnecessary metadata fetch per file.RESERVED_COLUMNSdeduplication:schema.pyimportsRESERVED_COLUMNSfromnemo_curator.tasks.interleavedinstead of redefining it.multimodal_mint1t_benchmark.pyextended with--reader-type,--writer-format,--no-filter,--reader-exclude-fields, and more.benchmarking/scripts/utils.pygainscollect_webdataset_output_metrics()andposition_countsvalidation.fsspec>=2024.12.0,s3fs>=2024.12.0; newmultimodal_cpuoptional extra (Pillow + s3fs).Code size
nemo_curator/stages/interleaved/)tests/stages/interleaved/)Benchmark Results
Dataset: MINT-1T PDF CC-2024-18, 90 tar shards (10 GB), 7,489 samples → 75,843 rows
Hardware: 256 CPUs available,
RAY_NUM_CPUS=64Regression check — no filter, no materialization (RayData executor, local NVMe)
Test plan
_escape_keyinjectivity verified (parametrized test for%,.,/,:)_resolve_schemaschema override tests (nullable preservation, type override, both-set warning, both-None raises)InterleavedParquetReader.decompose()test (FilePartitioningStage + InterleavedParquetReaderStage)_build_global_range_indextest (local + memory backends)align_tablepassthrough-columnsafe=Truevs reserved-columnsafe=Falsebehaviormaterialize_on_read(no TIFF frame extraction path taken)on_materialize_errorall four modes (error/warn/drop_row/drop_sample) —test_base_writer.pyreconcile_schema/align_tableedge cases — large_binary, dictionary passthrough, nullable=False preservation, column reordering, overflow detectionwds.WebDatasetAPI compatibility — 4 sample varieties in a single tar: text-only, multi-image (JPEG+PNG), interleaved text+image, percent-encoded key