Add "_skipme" option to Lhotse Dataloading #11793

monica-sekoyan · 2025-01-08T22:30:38Z

What does this PR do ?

Lhotse Dataloading will ignore samples where "_skipme" is set to 1 if not instructed otherwise with ds.ignore_skipme=True

Collection: [common]

Changelog

Add specific line by line info of high level changes in this PR.

Usage

You can potentially add a usage example below

# Add a code snippet demonstrating how to use this

GitHub Actions CI

The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.

The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".

Before your PR is "Ready for review"

Pre checks:

Make sure you read and followed Contributor guidelines
Did you write any new necessary tests?
Did you add or update any necessary documentation?
Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
- Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

New Feature
Bugfix
Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

Related to # (issue)

Signed-off-by: Monica Sekoyan <[email protected]>

Signed-off-by: monica-sekoyan <[email protected]>

nemo/collections/common/data/lhotse/dataloader.py

Signed-off-by: Monica Sekoyan <[email protected]>

nemo/collections/common/data/lhotse/dataloader.py

pzelasko

Please add a unit test

pzelasko · 2025-01-09T18:57:06Z

nemo/collections/common/data/lhotse/dataloader.py

@@ -443,6 +445,14 @@ def get_lhotse_sampler_from_config(config, global_rank, world_size, tokenizer=No
    cuts, use_iterable_dataset = read_cutset_from_config(config)
    use_iterable_dataset = determine_use_iterable_dataset(use_iterable_dataset, config)

+    if config.drop_skipme:
+        cuts = cuts.filter(lambda cut: not cut.custom.pop("_skipme", False) if hasattr(cut, "custom") else True)


Suggested change

cuts = cuts.filter(lambda cut: not cut.custom.pop("_skipme", False) if hasattr(cut, "custom") else True)

cuts = cuts.filter(lambda cut: "_skipme" not in cut.custom if hasattr(cut, "custom") else True)

I suggest the following change: skip whenever this key is present. This would let us adopt the following convention: {"_skipme": "reason for skipping"}

@pzelasko , for consistency in manifest entries wouldn't it be better to have "_skipme" keys for every entry with values "False" or "0" for entries we do not need to skip and "reason for skipping" for entries we need to skip?

Signed-off-by: Monica Sekoyan <[email protected]>

Signed-off-by: monica-sekoyan <[email protected]>

nemo/collections/common/data/lhotse/sampling.py

tests/collections/common/test_lhotse_dataloading.py

nemo/collections/common/data/lhotse/cutset.py

Signed-off-by: Monica Sekoyan <[email protected]>

Signed-off-by: monica-sekoyan <[email protected]>

Signed-off-by: Monica Sekoyan <[email protected]>

…skipme

Signed-off-by: Monica Sekoyan <[email protected]>

Signed-off-by: monica-sekoyan <[email protected]>

github-actions · 2025-02-03T09:10:03Z

beep boop 🤖: 🙏 The following files have warnings. In case you are familiar with these, please try helping us to improve the code base.

Your code was analyzed with PyLint. The following annotations have been identified:

************* Module nemo.collections.common.data.lhotse.cutset
nemo/collections/common/data/lhotse/cutset.py:635:0: C0301: Line too long (139/119) (line-too-long)
nemo/collections/common/data/lhotse/cutset.py:69:0: C0115: Missing class docstring (missing-class-docstring)
nemo/collections/common/data/lhotse/cutset.py:208:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:229:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:244:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:263:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:278:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:294:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:301:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:347:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:446:0: C0116: Missing function or method docstring (missing-function-docstring)
nemo/collections/common/data/lhotse/cutset.py:495:0: C0116: Missing function or method docstring (missing-function-docstring)

-----------------------------------
Your code has been rated at 9.52/10

Mitigation guide:

Add sensible and useful docstrings to functions and methods
For trivial methods like getter/setters, consider adding # pylint: disable=C0116 inside the function itself
To disable multiple functions/methods at once, put a # pylint: disable=C0116 before the first and a # pylint: enable=C0116 after the last.

By applying these rules, we reduce the occurance of this message in future.

Thank you for improving NeMo's documentation!

github-actions · 2025-02-03T09:10:05Z

beep boop 🤖: 🚨 The following files must be fixed before merge!

Your code was analyzed with PyLint. The following annotations have been identified:

************* Module nemo.collections.common.data.lhotse.sampling
nemo/collections/common/data/lhotse/sampling.py:15:0: W0611: Unused import bisect (unused-import)
nemo/collections/common/data/lhotse/sampling.py:16:0: W0611: Unused import logging (unused-import)
nemo/collections/common/data/lhotse/sampling.py:18:0: W0611: Unused bisect_right imported from bisect (unused-import)

-----------------------------------
Your code has been rated at 9.83/10

Mitigation guide:

Add sensible and useful docstrings to functions and methods
For trivial methods like getter/setters, consider adding # pylint: disable=C0116 inside the function itself
To disable multiple functions/methods at once, put a # pylint: disable=C0116 before the first and a # pylint: enable=C0116 after the last.

By applying these rules, we reduce the occurance of this message in future.

Thank you for improving NeMo's documentation!

add skipme option

653e990

Signed-off-by: Monica Sekoyan <[email protected]>

monica-sekoyan requested review from pzelasko and nithinraok January 8, 2025 22:30

github-actions bot added the common label Jan 8, 2025

Apply isort and black reformatting

d6a3706

Signed-off-by: monica-sekoyan <[email protected]>

nithinraok reviewed Jan 9, 2025

View reviewed changes

nemo/collections/common/data/lhotse/dataloader.py Outdated Show resolved Hide resolved

nemo/collections/common/data/lhotse/dataloader.py Outdated Show resolved Hide resolved

changed parameter name

69477eb

Signed-off-by: Monica Sekoyan <[email protected]>

monica-sekoyan requested a review from nithinraok January 9, 2025 02:35

pzelasko reviewed Jan 9, 2025

View reviewed changes

nemo/collections/common/data/lhotse/dataloader.py Outdated Show resolved Hide resolved

pzelasko reviewed Jan 9, 2025

View reviewed changes

monica-sekoyan and others added 2 commits January 9, 2025 22:50

modified skipme filtering

a427776

Signed-off-by: Monica Sekoyan <[email protected]>

Apply isort and black reformatting

3d74fb6

Signed-off-by: monica-sekoyan <[email protected]>

monica-sekoyan requested a review from pzelasko January 10, 2025 06:53

pzelasko requested changes Jan 22, 2025

View reviewed changes

monica-sekoyan and others added 3 commits January 31, 2025 09:41

updated skipme filter and related tests

556ffda

Signed-off-by: Monica Sekoyan <[email protected]>

solved merge conflict

59bd28e

Signed-off-by: Monica Sekoyan <[email protected]>

Apply isort and black reformatting

63df8c5

Signed-off-by: monica-sekoyan <[email protected]>

monica-sekoyan requested a review from pzelasko January 31, 2025 17:48

monica-sekoyan and others added 4 commits February 3, 2025 00:46

simplify tests

8dd3004

Signed-off-by: Monica Sekoyan <[email protected]>

Merge branch 'main' of https://github.com/NVIDIA/NeMo into msekoyan/_…

9c95fd5

…skipme

resolve merge conlicts

c7ff566

Signed-off-by: Monica Sekoyan <[email protected]>

Apply isort and black reformatting

538fda2

Signed-off-by: monica-sekoyan <[email protected]>

Merge branch 'main' into msekoyan/_skipme

5bcf20a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "_skipme" option to Lhotse Dataloading #11793

Add "_skipme" option to Lhotse Dataloading #11793

monica-sekoyan commented Jan 8, 2025

pzelasko left a comment

pzelasko Jan 9, 2025

monica-sekoyan Jan 9, 2025

github-actions bot commented Feb 3, 2025

github-actions bot commented Feb 3, 2025

	cuts = cuts.filter(lambda cut: not cut.custom.pop("_skipme", False) if hasattr(cut, "custom") else True)
	cuts = cuts.filter(lambda cut: "_skipme" not in cut.custom if hasattr(cut, "custom") else True)

Add "_skipme" option to Lhotse Dataloading #11793

Are you sure you want to change the base?

Add "_skipme" option to Lhotse Dataloading #11793

Conversation

monica-sekoyan commented Jan 8, 2025

What does this PR do ?

Changelog

Usage

GitHub Actions CI

Before your PR is "Ready for review"

Who can review?

Additional Information

pzelasko left a comment

Choose a reason for hiding this comment

pzelasko Jan 9, 2025

Choose a reason for hiding this comment

monica-sekoyan Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025

github-actions bot commented Feb 3, 2025