Integrate OpusCleaner #163

eu9ene · 2023-08-18T23:45:07Z

The first pass on integrating OpusCleaner that's meant to replace our cleaning scripts.

Included in this PR:

cleaning parallel corpus
default config that is similar to our current config
integration with Snakemake
integration with TaskCluster
run in CI
tested with a full pipeline production config and a default filter on en-ru and Snakemake (all completed except CCMatrix)

Out of scope:

cleaning monolingual data
reviewing deeper and improving the default config
creating the custom filters for a language pair using OpusCleaner UI
running experiments to see how the new filtering and custom filters affect translation quality
migrating the repository of configs to a different place (we might not want to include all the custom configs in the TC artifacts and it seems resources doesn't support directories or wildcards)

fixes #160

Discovered unfixed issues:

opuscleaner-clean hangs on CCMatrix hplt-project/OpusCleaner#116
TaskCluster doesn't redirect stderr to stdout like we do in snakemake, so OpusCleaner logs are not visible by default. I had to redirect manually to a file which is ok but makes it less visible when running manually. We should probably think of a more generic solution because some tools that work with stdin/stdout write log to stderr.

# Conflicts: # Makefile

gregtatum

Suggestion (docs): Since we're adding new tools to the pipeline, it would be nice to start the practice of documenting them a bit more. It would be nice to get a high-level markdown file documenting what is going on with OpusCleaner and why we use it. It would be nice to explain how we layer the configs, and how to start writing and using your own. For this PR it would be nice to at least start this documentation process, and I think it would be fine to file a follow-up to add more docs from there.

In conclusion: I'm going to mark this as approved on my end as I don't see any reason to block it. If you add substantial changes it would be nice for me to look at it again, so feel free to re-flag me for review.

Again, I use https://conventionalcomments.org/ for my reviews, so things like "Thought" is not something that required action, while "Suggestion" is. I use "Thought" when it's nice to have a conversation or when I'm thinking about the bigger problem.

gregtatum · 2023-09-25T17:07:05Z

Makefile

@@ -116,6 +116,19 @@ tensorboard:
 	tensorboard --logdir=$(MODELS) --host=0.0.0.0 &
 	python utils/tb_log_parser.py --prefix=

+
+install-opuscleaner:


Suggestion(docs): Since the Makefile is a developer-facing entry point into the code, it would be nice to provide a short high-level piece of documentation describing what each these actions are. It would also be nice to mention what OpusCleaner is for someone onboarding to the codebase.

OpusCleaner is a data cleaner for training data. https://github.com/hplt-project/OpusCleaner

gregtatum · 2023-09-25T17:08:19Z

pipeline/clean/opuscleaner/configs/default.filters.json

@@ -0,0 +1,91 @@
+{


Thought: It's a shame this is JSON as we can't add comments about the configuration and why certain defaults were chosen.

Does this have to adhere to a specific JSON schema? We could have an optional comment/description field since we can't have comments.

I created this filter template using the OpusCleaner UI tool. It should be compatible with the filters. I don't know if they have a schema somewhere to validate it. This would be a good follow-up task to validate the schema before running things.

gregtatum · 2023-09-25T17:10:44Z

pipeline/clean/opuscleaner/configs/remove_frequent_patterns.txt

@@ -0,0 +1,9 @@
+# Starting text in a lot of rows in OpenSubtitles, not everytime in both source and targets


Suggestion: This first comment doesn't make sense to me. It would be nice to re-phrase it using a more complete sentence.

The same goes for "Text in Czech Bible translations that is not in the source"

gregtatum · 2023-09-25T17:10:56Z

pipeline/clean/opuscleaner/configs/remove_frequent_patterns.txt

@@ -0,0 +1,9 @@
+# Starting text in a lot of rows in OpenSubtitles, not everytime in both source and targets


Question: What is this file exactly?

This file is required for one of the filters and it is a copy-paste from opus-cleaner. https://github.com/hplt-project/OpusCleaner/blob/main/opuscleaner/filters/remove_frequent_patterns.txt

gregtatum · 2023-09-25T17:11:13Z

pipeline/clean/opuscleaner/generate_filters.py

@@ -0,0 +1,78 @@
+"""
+Generates filter config for a dataset based on defaults to use in OpusCleaner


Praise: Thanks for the docs!

gregtatum · 2023-09-25T17:24:57Z

pipeline/clean/opuscleaner/clean-corpus.sh

@@ -0,0 +1,61 @@
+#!/bin/bash


Thought: This command is testable as well, but I'm not going to request a test here at this time.

gregtatum · 2023-09-25T17:25:09Z

pipeline/clean/opuscleaner/clean-corpus.sh

@@ -0,0 +1,61 @@
+#!/bin/bash


Question: Is there a reason this is in bash rather than python?

This is a very simple script that just calls CLI tools and does some IO with them, so I didn't see any reason to write it in Python. Also, we don't have a pattern setup of how to write such things in Python efficiently. It shouldn't be hard, but it's just way faster to write this in bash. As a follow up you can try refactoring this later in Python to compare the complexity and the benefits of such scripts.

gregtatum · 2023-09-25T17:27:50Z

pipeline/clean/opuscleaner/clean-corpus.sh

+  --batch-size=50000 \
+  --input=- \
+  "${filter_path}" "${SRC}" "${TRG}" \
+  2> "${output_prefix}.opus_filter.log" |


Question: I guess this outputs the opus log to the artifacts directory.

Yes, sometimes the opuscleaner writes to stderr that we can't see in the artifacts. We should address this issue on a higher level probably.

gregtatum · 2023-09-25T17:28:25Z

taskcluster/ci/clean-corpus/kind.yml

+                - >-
+                    pip install -r $VCS_PATH/pipeline/clean/requirements/clean.txt &&
+                    if [ ${USE_OPUSCLEANER} = "true" ]; then dir="clean/opuscleaner"; else dir="clean"; fi &&
+                    $VCS_PATH/pipeline/${dir}/clean-corpus.sh $MOZ_FETCHES_DIR/{dataset_sanitized} /builds/worker/artifacts/{dataset_sanitized} auto {dataset}


Suggestion: This will fail the new yamllint rules as beeing too long. It's also not very readable as it is written.

Let's merge the linting and then fix all the issues it finds.

gregtatum · 2023-09-25T17:29:52Z

taskcluster/ci/clean-corpus/kind.yml

+                - >-
+                    pip install -r $VCS_PATH/pipeline/clean/requirements/clean.txt &&
+                    if [ ${USE_OPUSCLEANER} = "true" ]; then dir="clean/opuscleaner"; else dir="clean"; fi &&
+                    $VCS_PATH/pipeline/${dir}/clean-corpus.sh $MOZ_FETCHES_DIR/{dataset_sanitized} /builds/worker/artifacts/{dataset_sanitized} auto {dataset}


Thought: This is another example where the bash script vs python argparse makes it difficult to understand how everything is wired together. With named arguments this would be much easier to understand.

eu9ene · 2023-09-25T18:20:46Z

Suggestion (docs): Since we're adding new tools to the pipeline, it would be nice to start the practice of documenting them a bit more. It would be nice to get a high-level markdown file documenting what is going on with OpusCleaner and why we use it. It would be nice to explain how we layer the configs, and how to start writing and using your own. For this PR it would be nice to at least start this documentation process, and I think it would be fine to file a follow-up to add more docs from there.

In conclusion: I'm going to mark this as approved on my end as I don't see any reason to block it. If you add substantial changes it would be nice for me to look at it again, so feel free to re-flag me for review.

Again, I use https://conventionalcomments.org/ for my reviews, so things like "Thought" is not something that required action, while "Suggestion" is. I use "Thought" when it's nice to have a conversation or when I'm thinking about the bigger problem.

I have refactoring of the docs on my list and filed an issue for this.

Also, this PR was meant as just an initial integration and to hand over the work as we discussed. The follow-up work that will be required to complete this is listed here. We can refactor things there and experiment with alternative approaches.

eu9ene · 2023-09-26T22:25:22Z

I added the docs and comments according to the suggestions and fixed an issue with the custom config. Nothing major, so merging.

eu9ene added 30 commits August 16, 2023 17:26

Initial integration of opus cleaner

fac320c

Support custom filters

c5f2332

Use opus cleaner in pipeline

0910cd5

Fix env

3df375b

Fix filter generation

966d3a1

Merge branch 'main' into opus_cleaner

50bceb1

Add more rules

d3ba834

Fix elrc filter

3f51c1c

Fix env

6554e50

Fix frequent patterns filter

b01df51

Switch to reading from stdin

40a82d6

Add a feature flag for opus cleaner

d691985

Fix condition

216a485

Add extra test for non empty files

6b62166

Merge branch 'main' into opus_cleaner

cbc86f0

# Conflicts: # Makefile

Integrate with TC

21b0025

Run linter

831ed74

Fix step config

8c98421

Fix step config

3316feb

Fix step config

25026e4

Fix step config

85dce3a

Merge branch 'main' into opus_cleaner

5972407

Fix command

929fe8e

Merge remote-tracking branch 'origin/opus_cleaner' into opus_cleaner

8787191

Fix path

80f04d0

Update OpusCleaner

84060a7

Remove warning

1d7c1ad

Log filtered length

78777a6

Add opuscleaner logs

b655c8d

Add comments

afdbec8

eu9ene added 4 commits September 22, 2023 14:09

Merge branch 'main' into opus_cleaner

aea4901

Extract function

82599ef

Change the CI target back

6bd44d6

Fix file path

b06107c

eu9ene marked this pull request as ready for review September 22, 2023 22:18

eu9ene requested a review from a team as a code owner September 22, 2023 22:18

eu9ene requested review from hneiva, gregtatum and gabrielBusta and removed request for hneiva September 22, 2023 22:18

gregtatum approved these changes Sep 25, 2023

View reviewed changes

gabrielBusta approved these changes Sep 25, 2023

View reviewed changes

eu9ene mentioned this pull request Sep 25, 2023

Add full support of the custom filters for OpusCleaner #210

Closed

eu9ene added 14 commits September 26, 2023 11:33

Replace conda with poetry

cb13c31

Add doc

53b82ad

Add more comments

cb6c414

Rename example filter

9377a63

Test corpus

0ddcd8d

Fix filter name

b73e1ad

Use opus dataset instead of mtdata

282284d

Make CI faster

09d8ee9

Add sections to makefile

82aa769

Fix custom filter search

20fdab4

Redirect stderr to stdout

81f3482

Fix usage of custom config

b24d255

Fix config name

aa48442

Change back to all

02d5ebd

eu9ene merged commit e9102a3 into mozilla:main Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate OpusCleaner #163

Integrate OpusCleaner #163

eu9ene commented Aug 18, 2023 •

edited

Loading

gregtatum left a comment

gregtatum Sep 25, 2023

gregtatum Sep 25, 2023

gabrielBusta Sep 25, 2023

eu9ene Sep 25, 2023

gregtatum Sep 25, 2023

gregtatum Sep 25, 2023

eu9ene Sep 25, 2023

gregtatum Sep 25, 2023

gregtatum Sep 25, 2023

gregtatum Sep 25, 2023

eu9ene Sep 25, 2023

gregtatum Sep 25, 2023

eu9ene Sep 25, 2023

gregtatum Sep 25, 2023

eu9ene Sep 25, 2023

gregtatum Sep 25, 2023

eu9ene commented Sep 25, 2023

eu9ene commented Sep 26, 2023

		@@ -0,0 +1,9 @@
		# Starting text in a lot of rows in OpenSubtitles, not everytime in both source and targets

		@@ -0,0 +1,78 @@
		"""
		Generates filter config for a dataset based on defaults to use in OpusCleaner

Integrate OpusCleaner #163

Integrate OpusCleaner #163

Conversation

eu9ene commented Aug 18, 2023 • edited Loading

gregtatum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eu9ene commented Sep 25, 2023

eu9ene commented Sep 26, 2023

eu9ene commented Aug 18, 2023 •

edited

Loading