Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create separate files for each deduplication class #409

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sarahyurick
Copy link
Collaborator

Re-opening #389 because of merge issues.

Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Sarah Yurick <[email protected]>
@sarahyurick sarahyurick added the gpuci Run GPU CI/CD on PR label Dec 13, 2024
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Generally in favor of this.

Would also check and update the api docs if needed: https://github.com/NVIDIA/NeMo-Curator/blob/main/docs/user-guide/api/deduplication.rst (not sure if it is or not).

Would also update any nemo-ci scripts if needed to accomodate the changes here.

nemo_curator/modules/__init__.py Outdated Show resolved Hide resolved
@ayushdg
Copy link
Collaborator

ayushdg commented Dec 20, 2024

cc: @VibhuJawa Could you take a look for semdedup as well?

@sarahyurick sarahyurick added gpuci Run GPU CI/CD on PR and removed gpuci Run GPU CI/CD on PR labels Dec 20, 2024
@sarahyurick sarahyurick requested a review from ayushdg December 20, 2024 18:47
@sarahyurick sarahyurick added gpuci Run GPU CI/CD on PR and removed gpuci Run GPU CI/CD on PR labels Jan 3, 2025
Copy link
Collaborator

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

Reviwed the semdedup functions, my only nit pick is to keep the module level import for stuff like ClusteringModel , EmbeddingCreator etc because they can also be generic in nature so it is a easier mental model.

@@ -18,10 +18,10 @@

import dask_cudf

from nemo_curator import ClusteringModel
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep this as semnatic dedup specific import (because clustering can be generic). So something like:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like EmbeddingCreator, ClusteringModel, and SemanticClusterLevelDedup are currently available as generic imports in NeMo Curator: https://github.com/NVIDIA/NeMo-Curator/blob/main/docs/user-guide/api/deduplication.rst

Should I change it to only be importable via nemo_curator.semantic_dedup instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpuci Run GPU CI/CD on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants