Skip to content

Filtering empty text examples - #1222

Merged
artemru merged 6 commits into
main_w2v2_pretrainingfrom
filtering_empty_text_examples
Jul 3, 2025
Merged

Filtering empty text examples#1222
artemru merged 6 commits into
main_w2v2_pretrainingfrom
filtering_empty_text_examples

Conversation

@artemru

@artemru artemru commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

What does this PR do? Please describe:
Removing empty text examples in ASR dataloaders

  • This requires to move tokenization before bucketing!

Does your PR introduce any breaking changes? If yes, please list them:
List of all backwards-incompatible changes.

Check list:

  • Was the content of this PR discussed and approved via a GitHub issue? (no need for typos or documentation improvements)
  • Did you read the contributor guideline?
  • Did you make sure that your PR does only one thing instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (no need for typos, documentation, or minor internal changes)

@artemru
artemru requested review from cbalioglu and zyaoj as code owners July 2, 2025 08:39
@artemru
artemru requested a review from mattsetz July 2, 2025 08:39
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 2, 2025
@artemru
artemru removed the request for review from cbalioglu July 2, 2025 08:40
@artemru
artemru changed the base branch from main to main_w2v2_pretraining July 2, 2025 08:40

@mattsetz mattsetz left a comment

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.

looks good! do we expect this will slow down dataloading time to tokenize examples one by one? or maybe its minimal bc the tokenization is so fast?

@staticmethod
def add_tokenization_pipeline(
builder: DataPipelineBuilder,
tokenizer: TextTokenizer,

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.

we can add another arg here remove_unk to optionally remove all UNKs tokens from transcript. we should default this to False but it can be something that we experiment with.

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.

also, maybe we can log the number of UNKs (or % of num_unks/total_tokens within a sample/batch) in weights and biases? its useful for us to know if our dataset contains too many UNKs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

also, maybe we can log the number of UNKs (or % of num_unks/total_tokens within a sample/batch) in weights and biases? its useful for us to know if our dataset contains too many UNKs.

it's possible (requires a bit to propagate some metric bags to data reader) but for now we can just do in a adhoc analysis

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can add another arg here remove_unk to optionally remove all UNKs tokens from transcript. we should default this to False but it can be something that we experiment with.

done !

Comment thread src/fairseq2/datasets/asr.py
@artemru

artemru commented Jul 3, 2025

Copy link
Copy Markdown
Contributor Author

looks good! do we expect this will slow down dataloading time to tokenize examples one by one? or maybe its minimal bc the tokenization is so fast?

there'll not be slowdown in the iterations (still need to do tokenization just in different order), but it will slightly increase the starting time. Tokenization is rather fast (>200k tokens /sec)

@artemru
artemru merged commit d879a11 into main_w2v2_pretraining Jul 3, 2025
15 checks passed
@artemru
artemru deleted the filtering_empty_text_examples branch July 3, 2025 09:34
cirquit pushed a commit to cirquit/fairseq2 that referenced this pull request Jul 17, 2025
* filter empty text examples asr dataloading

* comment

* flake8

* early exit

* isort

* rm UNK with option

---------

Co-authored-by: Artyom Kozhevnikov <artyomko@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants