Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,10 @@ def read_and_insert(
) -> list[FileGroupTask]:
"""Read and insert tasks into the shuffler."""
insert_kwargs = {"band_range": band_range} if band_range is not None else {}
results = []
for task in tasks:
results.append(self.stage.read_and_insert(task, **insert_kwargs))
return results
if hasattr(self.stage, "read_and_insert_batch"):
return self.stage.read_and_insert_batch(tasks, **insert_kwargs)
Comment on lines +132 to +133

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

band_range kwarg forwarded to read_and_insert_batch which doesn't accept it

When band_range is not None, the executor builds insert_kwargs = {"band_range": band_range} and passes it via **insert_kwargs to self.stage.read_and_insert_batch(tasks, **insert_kwargs). However, ExactDuplicateIdentification.read_and_insert_batch only accepts tasks and will raise a TypeError if band_range is ever non-None.

This is currently safe because band_range is only non-None for LSH stages, and LSH stages don't implement read_and_insert_batch yet. However, if read_and_insert_batch is added to any stage that also uses band_range (e.g. an LSH stage), it will silently break. Consider adding band_range as an optional parameter (defaulting to None) to read_and_insert_batch, or using **kwargs:

def read_and_insert_batch(self, tasks: list[FileGroupTask], band_range: tuple[int, int] | None = None) -> list[FileGroupTask]:

else:
return [self.stage.read_and_insert(task, **insert_kwargs) for task in tasks]

def insert_finished(self) -> None:
"""Finish the insertion phase and trigger shuffle."""
Expand Down
103 changes: 85 additions & 18 deletions nemo_curator/stages/deduplication/exact/identification.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

from typing import TYPE_CHECKING, Any, Literal

import cudf

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cudf promoted to hard runtime import

Previously cudf was guarded under TYPE_CHECKING so it was only imported by type checkers. Now it is a top-level runtime import (needed for cudf.concat in read_and_insert_batch). This is a breaking change for any code path that imports this module in a non-GPU environment — the import will raise ModuleNotFoundError even if the class is never instantiated. The test file already handles this with suppress(ImportError), but workflow.py imports ExactDuplicateIdentification unconditionally.

If non-GPU imports of the package need to remain possible, consider lazy-importing cudf inside the method that needs it:

def read_and_insert_batch(self, tasks: list[FileGroupTask]) -> list[FileGroupTask]:
    import cudf  # only needed at runtime for concat
    ...


from nemo_curator.stages.deduplication.id_generator import CURATOR_DEDUP_ID_STR, get_id_generator_actor
from nemo_curator.stages.deduplication.io_utils import DeduplicationIO
from nemo_curator.stages.deduplication.shuffle_utils.rapidsmpf_shuffler import pylibcudf_to_cudf_dataframe
Expand All @@ -22,8 +24,6 @@
from nemo_curator.utils.file_utils import get_fs

if TYPE_CHECKING:
import cudf

from nemo_curator.backends.base import WorkerMetadata


Expand Down Expand Up @@ -145,32 +145,99 @@ def inputs(self) -> tuple[list[str], list[str]]:
def outputs(self) -> tuple[list[str], list[str]]:
return (["data"], [])

def read_and_insert(self, task: FileGroupTask) -> FileGroupTask:
def _hash_and_insert(self, df: "cudf.DataFrame") -> None:
"""Hash the text field and insert into the shuffle actor.

Parameters
----------
df
DataFrame containing the id_field and text_field columns.
"""
self._check_actor_obj()
hashed_df = df[[self.id_field]]
hashed_df[EXACT_DUPLICATE_GROUP_FIELD] = df[self.text_field].hash_values(method="md5")
Comment on lines +157 to +158

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SettingWithCopyWarning / silent no-op risk on cuDF slice

hashed_df = df[[self.id_field]]
hashed_df[EXACT_DUPLICATE_GROUP_FIELD] = df[self.text_field].hash_values(method="md5")

df[[col]] returns a copy in cuDF (the double-bracket selection), so the column assignment does land on hashed_df and not on df. However, depending on the cuDF version this can raise a SettingWithCopyWarning. The safer and clearer idiom is to build the result in one shot:

Suggested change
hashed_df = df[[self.id_field]]
hashed_df[EXACT_DUPLICATE_GROUP_FIELD] = df[self.text_field].hash_values(method="md5")
hashed_df = cudf.DataFrame({
self.id_field: df[self.id_field],
EXACT_DUPLICATE_GROUP_FIELD: df[self.text_field].hash_values(method="md5"),
})

This avoids any copy-on-write ambiguity and is explicit about intent.

self.output_columns = list(hashed_df.columns)
self._actor_obj.insert_chunk(hashed_df, self.output_columns)

if self.assign_id_field and self.id_generator is None:
msg = "ID generator not initialized. Call setup() first."
raise RuntimeError(msg)
def _read_files(self, filepaths: list[str]) -> "cudf.DataFrame":
"""Read files and return a DataFrame.

Parameters
----------
filepaths
List of file paths to read.

Returns
-------
cudf.DataFrame
DataFrame containing the id_field and text_field columns.
"""
input_columns = [self.text_field] if self.assign_id_field else [self.text_field, self.id_field]
if self.input_filetype == "jsonl":
df = self.read_jsonl(
filepath=task.data, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs
)
read_func = self.read_jsonl
elif self.input_filetype == "parquet":
df = self.read_parquet(
filepath=task.data, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs
)
read_func = self.read_parquet
else:
msg = f"Unsupported input filetype: {self.input_filetype}"
raise ValueError(msg)
return read_func(filepaths, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs)

Comment on lines +159 to 184

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to _check_actor_obj() is deferred until _hash_and_insert, which runs after file reads (lines 190–197) and cudf.concat() (line 198). If the actor is not initialized, this check fails only after expensive GPU I/O and memory operations have completed.

Move the check to the top of read_and_insert_batch to catch misconfiguration immediately:

Suggested change
self.output_columns = list(hashed_df.columns)
self._actor_obj.insert_chunk(hashed_df, self.output_columns)
if self.assign_id_field and self.id_generator is None:
msg = "ID generator not initialized. Call setup() first."
raise RuntimeError(msg)
def _read_files(self, filepaths: list[str]) -> "cudf.DataFrame":
"""Read files and return a DataFrame.
Parameters
----------
filepaths
List of file paths to read.
Returns
-------
cudf.DataFrame
DataFrame containing the id_field and text_field columns.
"""
input_columns = [self.text_field] if self.assign_id_field else [self.text_field, self.id_field]
if self.input_filetype == "jsonl":
df = self.read_jsonl(
filepath=task.data, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs
)
read_func = self.read_jsonl
elif self.input_filetype == "parquet":
df = self.read_parquet(
filepath=task.data, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs
)
read_func = self.read_parquet
else:
msg = f"Unsupported input filetype: {self.input_filetype}"
raise ValueError(msg)
return read_func(filepaths, columns=input_columns, assign_id=self.assign_id_field, **self.read_kwargs)
def read_and_insert_batch(self, tasks: list[FileGroupTask]) -> list[FileGroupTask]:
"""Batch process multiple file group tasks for exact deduplication.
This method reads all files from all tasks, concatenates them (if needed),
hashes the text field using MD5, and inserts into the shuffle actor for
deduplication. Processing tasks in batches significantly improves
throughput by reducing actor call overhead and enabling more efficient
GPU operations.
Parameters
----------
tasks
List of FileGroupTask objects containing files to process.
Must contain at least one task.
Returns
-------
list[FileGroupTask]
The input tasks unchanged. The actual deduplication results are
written through the shuffle actor as a side effect.
Raises
------
RuntimeError
If ID generator is not initialized when assign_id is True.
"""
self._check_actor_obj()
if self.assign_id_field and self.id_generator is None:

hashed_df = df[[self.id_field]]
hashed_df[EXACT_DUPLICATE_GROUP_FIELD] = df[self.text_field].hash_values(method="md5")
self.output_columns = list(hashed_df.columns)
self.dataset_name = task.dataset_name
def read_and_insert_batch(self, tasks: list[FileGroupTask]) -> list[FileGroupTask]:
"""Batch process multiple file group tasks for exact deduplication.

self._actor_obj.insert_chunk(hashed_df, self.output_columns)
return task
This method reads all files from all tasks, concatenates them (if needed),
hashes the text field using MD5, and inserts into the shuffle actor for
deduplication. Processing tasks in batches significantly improves
throughput by reducing actor call overhead and enabling more efficient
GPU operations.

Parameters
----------
tasks
List of FileGroupTask objects containing files to process.
Must contain at least one task.

Returns
-------
list[FileGroupTask]
The input tasks unchanged. The actual deduplication results are
written through the shuffle actor as a side effect.

Raises
------
RuntimeError
If ID generator is not initialized when assign_id is True.
"""
if self.assign_id_field and self.id_generator is None:
msg = "ID generator not initialized. Call setup() first."
raise RuntimeError(msg)

# Optimize for single-task batch to avoid unnecessary concat overhead
if len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
Comment on lines +218 to +221

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation for empty tasks list. The docstring states "Must contain at least one task" but there's no check. If an empty list is passed, this will crash with IndexError.

Suggested change
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
# Optimize for single-task batch to avoid unnecessary concat overhead
if not tasks:
msg = "tasks list must contain at least one task"
raise ValueError(msg)
elif len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When batching multiple tasks with different dataset_name values, only the first task's name is used. Consider checking if all tasks have the same dataset name, or documenting this behavior.

Comment on lines +219 to +221

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When batching multiple tasks with different dataset_name values, only the first task's name is used. This could cause issues if tasks from different datasets are batched together. Consider validating that all tasks have the same dataset name:

Suggested change
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
# Validate all tasks have same dataset name
dataset_names = {task.dataset_name for task in tasks}
if len(dataset_names) > 1:
msg = f"All tasks in a batch must have the same dataset_name, found: {dataset_names}"
raise ValueError(msg)
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name

Comment on lines +215 to +221

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty tasks list will cause IndexError on line 217/222. Add validation:

Suggested change
if len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
if not tasks:
msg = "tasks list must contain at least one task"
raise ValueError(msg)
# Optimize for single-task batch to avoid unnecessary concat overhead
if len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name

Comment on lines +218 to +221

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tasks' files loaded into GPU memory simultaneously before concat

When len(tasks) > 1, the code reads every task's data into a separate cuDF DataFrame via a synchronous list comprehension, holds all of them in GPU memory at once, and only then calls cudf.concat. For a batch_size of N with input_blocksize of B, the peak GPU memory required is N × B before any of it is released.

This is the correct design trade-off for throughput, but it is not documented and can silently cause GPU OOM errors for callers who set a large identification_batchsize without accounting for the multiplicative memory impact. At minimum, the docstring for read_and_insert_batch and identification_batchsize should warn about peak memory usage.


self._hash_and_insert(df)
return tasks
Comment on lines +185 to +224

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation for empty tasks list. If an empty list is passed, line 217 will cause an IndexError when accessing tasks[0].

Suggested change
def read_and_insert_batch(self, tasks: list[FileGroupTask]) -> list[FileGroupTask]:
"""Batch process multiple file group tasks for exact deduplication.
self._actor_obj.insert_chunk(hashed_df, self.output_columns)
return task
This method reads all files from all tasks, concatenates them (if needed),
hashes the text field using MD5, and inserts into the shuffle actor for
deduplication. Processing tasks in batches significantly improves
throughput by reducing actor call overhead and enabling more efficient
GPU operations.
Parameters
----------
tasks
List of FileGroupTask objects containing files to process.
Must contain at least one task.
Returns
-------
list[FileGroupTask]
The input tasks unchanged. The actual deduplication results are
written through the shuffle actor as a side effect.
Raises
------
RuntimeError
If ID generator is not initialized when assign_id is True.
"""
if self.assign_id_field and self.id_generator is None:
msg = "ID generator not initialized. Call setup() first."
raise RuntimeError(msg)
# Optimize for single-task batch to avoid unnecessary concat overhead
if len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
self._hash_and_insert(df)
return tasks
def read_and_insert_batch(self, tasks: list[FileGroupTask]) -> list[FileGroupTask]:
"""Batch process multiple file group tasks for exact deduplication.
This method reads all files from all tasks, concatenates them (if needed),
hashes the text field using MD5, and inserts into the shuffle actor for
deduplication. Processing tasks in batches significantly improves
throughput by reducing actor call overhead and enabling more efficient
GPU operations.
Parameters
----------
tasks
List of FileGroupTask objects containing files to process.
Must contain at least one task.
Returns
-------
list[FileGroupTask]
The input tasks unchanged. The actual deduplication results are
written through the shuffle actor as a side effect.
Raises
------
RuntimeError
If ID generator is not initialized when assign_id is True.
ValueError
If tasks list is empty.
"""
if not tasks:
msg = "tasks list must contain at least one task"
raise ValueError(msg)
if self.assign_id_field and self.id_generator is None:
msg = "ID generator not initialized. Call setup() first."
raise RuntimeError(msg)
# Optimize for single-task batch to avoid unnecessary concat overhead
if len(tasks) == 1:
df = self._read_files(tasks[0].data)
self.dataset_name = tasks[0].dataset_name
else:
dfs = [self._read_files(task.data) for task in tasks]
df = cudf.concat(dfs, ignore_index=True)
self.dataset_name = tasks[0].dataset_name
self._hash_and_insert(df)
return tasks


def read_and_insert(self, task: FileGroupTask) -> FileGroupTask:
"""Single task processing is not supported.

This stage requires batch processing via read_and_insert_batch for
optimal performance. The shuffle adapter will automatically route
tasks to the batch method.

Raises
------
NotImplementedError
Always raised as this stage only supports batch processing.
"""
msg = "ExactDuplicateIdentification only supports batch processing via read_and_insert_batch"
raise NotImplementedError(msg)
Comment on lines +226 to +235

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback to read_and_insert will always fail since this method now raises NotImplementedError. This creates a breaking change for any code that might call read_and_insert directly outside the adapter context.

Suggested change
def read_and_insert(self, task: FileGroupTask) -> FileGroupTask:
"""Single task processing is not supported.
This stage requires batch processing via read_and_insert_batch for
optimal performance. The shuffle adapter will automatically route
tasks to the batch method.
Raises
------
NotImplementedError
Always raised as this stage only supports batch processing.
"""
msg = "ExactDuplicateIdentification only supports batch processing via read_and_insert_batch"
raise NotImplementedError(msg)
def read_and_insert(self, task: FileGroupTask) -> FileGroupTask:
"""Process a single task by delegating to batch method.
Parameters
----------
task
Single FileGroupTask to process.
Returns
-------
FileGroupTask
The input task unchanged.
"""
return self.read_and_insert_batch([task])[0]


def insert_finished(self) -> None:
super().insert_finished()
Expand Down
12 changes: 8 additions & 4 deletions tests/stages/deduplication/exact/test_identification.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,14 @@ def exact_no_dedup_data_jsonl(tmp_path: Path) -> list[FileGroupTask]:
@pytest.mark.gpu
@pytest.mark.usefixtures("ray_client_with_id_generator")
class TestExactDuplicates:
@pytest.mark.parametrize("assign_id", [True, False])
@pytest.mark.parametrize("total_nparts", [2, 4])
@pytest.mark.parametrize(("assign_id", "total_nparts", "batch_size"), [(False, 2, 1), (True, 4, 5)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage reduced from 4 combinations (2 assign_id × 2 total_nparts) to just 2 specific combinations. Consider restoring full combinatorial testing or add more edge cases like (True, 2, 1) and (False, 4, 5)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

def test_dup(
self, exact_dedup_data_parquet: list[FileGroupTask], tmpdir: Path, total_nparts: int, assign_id: bool
self,
exact_dedup_data_parquet: list[FileGroupTask],
tmpdir: Path,
total_nparts: int,
assign_id: bool,
batch_size: int,
) -> None:
stage = ExactDuplicateIdentification(
text_field="text",
Expand All @@ -133,7 +137,7 @@ def test_dup(
enable_statistics=False,
assign_id=assign_id,
id_field="id" if not assign_id else None,
)
).with_(batch_size=batch_size)
pipeline = Pipeline(name="test_exact_dedup", stages=[stage])
executor = RayActorPoolExecutor()
pipeline.run(executor, initial_tasks=exact_dedup_data_parquet)
Expand Down
Loading