Add Europarl-ST SRT recipe with LCMA-SRT#2096
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds the Europarl-ST LCMA-SRT recipe with dataset preparation, model and optimization modules, distributed training, multilingual decoding, evaluation reports, documentation, and staged launcher scripts. ChangesEuroparl-ST SRT recipe
Estimated code review effort: 5 (Critical) | ~150 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Collaborator
|
Thank you for your contribution! We are reviewing it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (10)
egs/europarl_st/SRT/lcma_srt/attention_decoder.py (1)
250-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop index
i.Iterate over layers directly to satisfy the linter.
♻️ Proposed tweak
- for i, mod in enumerate(self.layers): + for mod in self.layers: x = mod( x, attn_mask=attn_mask, memory=memory, memory_attn_mask=memory_attn_mask, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/attention_decoder.py` around lines 250 - 256, The loop in the decoder layer stack uses enumerate in `attention_decoder.py` but never references `i`, so update the iteration over `self.layers` in the decoder forward path to traverse the layers directly without capturing the unused index. Keep the call to each `mod` unchanged and remove only the unnecessary loop variable so the code satisfies the linter.Source: Linters/SAST tools
egs/europarl_st/SRT/lcma_srt/moe_adapter.py (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
int()aroundround().
round(x)(no ndigits) already returns anint, so the outerint(...)is unnecessary at both Line 52 and Line 71.♻️ Proposed tweak
- d_hidden = max(1, int(round(d_model * hidden_mult))) + d_hidden = max(1, round(d_model * hidden_mult))- router_hidden = max(16, int(round(d_model * router_mid_ratio))) + router_hidden = max(16, round(d_model * router_mid_ratio))Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/moe_adapter.py` at line 52, The `MoEAdapter` dimension calculations contain a redundant outer `int()` around `round()`, so update the hidden-size expressions in `MoEAdapter` to use `round(...)` directly and keep the `max(1, ...)` guard intact. Apply the same cleanup to both occurrences in the adapter initialization logic so the `d_hidden` and related size computation remain functionally identical but simpler.Source: Linters/SAST tools
egs/europarl_st/SRT/lcma_srt/train.py (2)
1848-1848: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead no-op parameter-aliasing loop.
for name in (...): setattr(params, f"{name}_asr", getattr(params, f"{name}_asr")) setattr(params, f"{name}_st", getattr(params, f"{name}_st"))This sets each attribute to itself and has no effect (argparse already stores dashed CLI flags as underscored attributes). Safe to remove.
Also applies to: 1856-1870
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/train.py` at line 1848, Remove the dead parameter-aliasing loop in run; the setattr calls in the name iteration are self-assignments and do nothing. Update run (and any related params setup around the ASR/ST argument handling) to rely on argparse’s existing underscored attributes instead of creating aliases, and delete the redundant block entirely.
2001-2007: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent fallback to LibriSpeech-named manifests if
--train-cuts-paths/--valid-cuts-pathsare omitted.When
args.train_cuts_paths/args.valid_cuts_pathsaren't provided, the code falls back tolibrispeech.train_all_shuf_cuts()/dev_clean_cuts()+dev_other_cuts(), which look forlibrispeech_cuts_train-all-shuf.jsonl.gz,librispeech_cuts_dev-clean.jsonl.gz, etc. — filenames that don't exist in this Europarl-ST recipe's manifest layout. All shipped launcher scripts pass the explicit paths so this isn't hit today, but runningtrain.pydirectly without them produces a confusingFileNotFoundErrordeep in a leftover LibriSpeech code path instead of a clear recipe-specific error.Consider asserting
train_cuts_paths/valid_cuts_pathsare set for this recipe, or raising a clearer error.Also applies to: 2092-2098
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/train.py` around lines 2001 - 2007, The fallback in the Europarl-ST manifest loading logic still uses LibriSpeech helper paths, which can trigger a confusing FileNotFoundError when train.py is run without explicit cut paths. Update the train/validation setup around the train_cuts and valid_cuts selection logic to require args.train_cuts_paths and args.valid_cuts_paths for this recipe, or raise a clear recipe-specific error before calling load_cuts_lazy or any librispeech.*_cuts helper. Keep the fix aligned with the existing parameter handling in the training entrypoint so the failure is immediate and informative.egs/europarl_st/SRT/lcma_srt/datamodule.py (2)
374-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate validation-dataloader methods; the used one has a placeholder name.
valid_dataloaders_gpt41(distributed-aware) andvalid_dataloaders(non-distributed) implement nearly the same logic. Onlyvalid_dataloaders_gpt41is called fromtrain.py, leavingvalid_dataloadersas dead code, and the "_gpt41" suffix looks like an unremoved AI-generation artifact rather than an intentional name.Consider removing the unused
valid_dataloadersand renamingvalid_dataloaders_gpt41to something meaningful (e.g.valid_dataloaders, replacing the old one).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/datamodule.py` around lines 374 - 465, The validation dataloader logic is duplicated, and the active method name valid_dataloaders_gpt41 is a placeholder that should be cleaned up. Keep the distributed-aware implementation in valid_dataloaders_gpt41, rename it to the intended public name valid_dataloaders so train.py continues to call the right method, and remove the unused older valid_dataloaders implementation to avoid dead code and confusion.
305-310: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
eval()on CLI-supplied--input-strategyis flagged as code-injection risk.Both Ruff (S307) and ast-grep flag
eval(self.args.input_strategy)(). Risk is low since this is an operator-supplied CLI arg, not attacker input, but a dict dispatch is trivial and removes the CWE-94 flag entirely.♻️ Suggested fix
+_INPUT_STRATEGIES = {"AudioSamples": AudioSamples, "PrecomputedFeatures": PrecomputedFeatures} + train = K2SpeechRecognitionDataset( - input_strategy=eval(self.args.input_strategy)(), + input_strategy=_INPUT_STRATEGIES[self.args.input_strategy](),Also applies to: 470-473
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/datamodule.py` around lines 305 - 310, Replace the eval(self.args.input_strategy)() usage in the datamodule setup with a safe dispatch map from allowed strategy names to the corresponding input strategy constructors, then instantiate the selected one in K2SpeechRecognitionDataset. Apply the same change in both places where this pattern appears so the input_strategy resolution is centralized, explicit, and no longer uses eval().Source: Linters/SAST tools
egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2m.sh (1)
24-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffLarge
TEST_CUTS_PATHSarray duplicated verbatim across decode launcher scripts.The same 73-entry array of language-pair manifest paths is repeated identically in
decode_lcma_srt.sh(and presumably other stage2 decode scripts). Extracting this into a shared file (sourced by both scripts, or generated from--srt-langs/--tgt-langs) would avoid drift if language pairs or manifest paths change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2m.sh` around lines 24 - 97, The TEST_CUTS_PATHS array is duplicated across decode launcher scripts, so update decode_hent_srt_m2m.sh to source the shared list instead of hardcoding it here. Move the language-pair manifest paths into a common script or generated helper used by decode_lcma_srt.sh and the other stage2 decode entry points, and keep the existing TEST_CUTS_PATHS symbol as the single source of truth to prevent drift.egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc.sh (1)
1-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting shared config across stage1 launcher scripts.
cr_ctc.shandcr_ctc_moe.sh(and the other stage1/stage2 launchers) are near-identical, differing only in a handful of MoE-related flags (asr-moe,entropy-reg-asr,num-experts-asr, etc.). Duplicating ~130 lines per variant makes future flag/path changes error-prone (must be updated consistently in every file). A shared sourced config (common paths, encoder dims, language lists) with per-variant overrides would reduce this maintenance burden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc.sh` around lines 1 - 135, The stage1 launcher duplicates a large shared configuration block, making flag and path updates easy to miss across variants. Refactor the common setup in cr_ctc.sh around the shared variable declarations and the torchrun invocation into a sourced/shared config script, then keep only the MoE-specific overrides in each launcher. Preserve the existing per-variant differences by parameterizing the shared values such as ASR_BPE_MODEL, TRAIN_CUTS_PATHS, encoder dimensions, and language lists, and have cr_ctc.sh consume that shared config instead of hardcoding everything locally.egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_s_bias.sh (1)
1-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared configuration to reduce duplication across launcher scripts.
All five stage1/stage2 scripts share ~90% identical content: the
log()function, environment exports, model architecture parameters, and most of thetorchruncommand. Only MoE config, data paths, task weights, and resume settings differ. A common sourced script (e.g.,common.sh) for shared variables and the log function would reduce maintenance burden and the risk of parameter drift between scripts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_s_bias.sh` around lines 1 - 135, The stage launcher script duplicates a large shared setup with the other stage1/stage2 scripts, including the log() helper, environment exports, architecture defaults, and most of the torchrun arguments. Extract the common configuration into a shared sourced script (for example a common.sh that defines log(), env vars, and shared training flags), then keep this script focused on its stage-specific differences such as data paths, MoE/task settings, and resume options.egs/europarl_st/SRT/lcma_srt/decode.py (1)
1521-1532: 🧹 Nitpick | 🔵 TrivialST WER results are never saved —
st_save_wer_resultscall is commented out.The function
st_save_wer_results(lines 1158–1201) is defined but never called. The commented-out block on lines 1527–1532 suggests this was intentionally disabled, but ST decoding results (results_dict_st) are not scored. If ST scoring is needed, uncomment and adjust the condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/decode.py` around lines 1521 - 1532, ST WER results are not being saved because the `st_save_wer_results` path in `decode.py` is commented out. Update the scoring flow in the block that already calls `asr_save_wer_results` so `st_save_wer_results` is invoked for ST decoding when appropriate, using `results_dict_st` and the existing `params.ast_use_asr_data` condition or the correct gating for ST scoring. Ensure the `st_save_wer_results` function is actually reached so ST results are recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@egs/europarl_st/SRT/lcma_srt/datamodule.py`:
- Around line 577-580: The test_cuts method in the Europarl-ST datamodule is
incorrectly prefixing self.args.test_name with self.args.manifest_dir, which
breaks decode.py when --test-name is already a full manifest path. Update
test_cuts to load the manifest directly from self.args.test_name without adding
manifest_dir, so the path resolution works for the stage2 decode scripts.
In `@egs/europarl_st/SRT/lcma_srt/decode.py`:
- Around line 1475-1485: The context-graph setup in the modified beam search
branch uses an undefined tokenizer variable and manually opens the context file.
Update the context loading block in the decoding logic to use the correct
SentencePiece instance already available in this module, likely sp_st or sp_asr
depending on the target text, and replace the raw open(...).readlines() usage
with a with statement so the file handle is closed properly. Keep the fix
localized to the context_graph-building path that reads params.context_file and
calls ContextGraph.build.
- Around line 1504-1512: The decode_dataset call in main is dropping the
configured decoding extras, so context biasing and LM-based rescoring never get
applied. Update the decode_dataset invocation to forward the context_graph, LM,
ngram_lm, and ngram_lm_scale objects that are built earlier in main, matching
the existing parameter names in decode_dataset so the configured decoding
behavior is preserved.
- Around line 533-754: The non-`modified_beam_search` branches in
`decode_one_batch` still reference single-task variables that no longer exist,
so update each decoding path to use the correct ASR or ST symbols from the
refactor. Fix `fast_beam_search`, `fast_beam_search_nbest_LG`,
`fast_beam_search_nbest`, `fast_beam_search_nbest_oracle`, `greedy_search`,
`modified_beam_search_lm_shallow_fusion`, `modified_beam_search_LODR`,
`modified_beam_search_lm_rescore`, `modified_beam_search_lm_rescore_LODR`, and
the fallback path so they use `asr_encoder_out`/`asr_encoder_out_lens`,
`st_encoder_out`/`st_encoder_out_lens`, `sp_asr`/`sp_st`, and
`asr_hyps`/`st_hyps` as appropriate. Also update the `main()` vocab size
validation to use the ASR/ST-specific vocab fields instead of
`params.vocab_size`, or remove unsupported decode methods if they are not meant
to be handled.
- Around line 1043-1061: The batch counting in decode.py can reference an
undefined ASR text list when ASR decoding is disabled. Update the control flow
around the `texts_asr`/`texts_st` handling in the main decode loop so `num_cuts`
does not depend on `texts_asr` unless `params.asr_decode` is true; use an
always-available batch size source such as `cut_ids` or guard the increment with
the same decode condition. Keep the fix aligned with the existing
`params.asr_decode`, `params.ast_decode`, and `num_cuts` logic in the decode
routine.
In `@egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2o.sh`:
- Around line 60-113: Set the target language token before calling decode.py in
decode_hent_srt_m2o.sh: tgt_lang_token is currently unset, so --lang-tgt becomes
empty and force_first_lang receives an invalid id. Add the same tgt_lang_token
assignment used in the other stage2 scripts (based on tgt_lang) near the
lang_pair_dir parsing, then keep the existing --lang-tgt "${tgt_lang_token}"
argument in the decode.py invocation.
In `@egs/europarl_st/SRT/lcma_srt/model.py`:
- Around line 350-357: `output_downsampling_factor_st` is only handled for the
values 1 and 2 in the ST branch, so invalid argparse values can leave `st_tnc`
and `st_lens` unset and fail later with an unclear error. Update the logic in
`model.py` around the `st_tnc` / `st_lens` assignment to add an `else` branch
that raises a clear `ValueError` for unsupported `output_downsampling_factor_st`
values, keeping the existing `self.enc_st.downsample_output` and
`self.output_downsampling` paths for the valid cases.
In `@egs/europarl_st/SRT/lcma_srt/train.py`:
- Around line 1391-1416: `compute_validation_loss` is skipping all non-zero
ranks, so validation only covers rank 0’s shard instead of the full distributed
validation set. Update this path to run evaluation on every rank that owns part
of `valid_dl` and then aggregate the resulting `MetricsTracker`/loss values
across ranks (for example via distributed reduction) before returning. Keep the
fix localized to `compute_validation_loss` and the surrounding validation metric
accumulation so `best_valid_loss` and TensorBoard `valid_` metrics reflect the
full dataset.
- Around line 687-729: The checkpoint loading in
`_load_model_weights_drop_st_head` (and the related
`_safe_load_ckpt_with_filter`) uses `torch.load(..., map_location="cpu")` on
potentially externally sourced files, which is unsafe. Update the loading flow
to use a weights-only path for untrusted pretrained checkpoints, while
preserving full checkpoint loading only for trusted same-run resume cases that
need `optimizer`, `scheduler`, `sampler`, or `params`. Keep the fix localized
around the existing checkpoint parsing and model state extraction so the
model-weight-only path remains compatible with
`_load_model_weights_drop_st_head`.
- Around line 822-911: The resume path in the checkpoint-loading logic is
dropping ast_moe_layer.router.* weights on every restart, which breaks same-run
recovery. Update the checkpoint selection in the resume helper around filename
and drop_prefixes so the router-prefix filter is only enabled for true
cross-stage warm starts via params.resume_from_checkpoint, and not for
continuation resumes via params.start_batch or params.start_epoch. Keep ordinary
crash recovery loading the full state, while preserving the existing filtered
load only when transitioning stages.
- Around line 1050-1053: The assert message in the text-validation block
contains hidden zero-width-space characters, so clean up the f-string in the
conditional that checks supervisions["text"] and len(tags) to use only normal
printable spaces and punctuation. Update the message in this assert so it
preserves the same wording and context but removes the invisible characters that
Ruff flags, especially around the "ASR languages" portion.
In `@egs/europarl_st/SRT/local/org_to_jsonl.py`:
- Around line 155-160: Add strict length checking to the two dict constructions
in org_to_jsonl.py by updating the zip() calls that build
segments_source_lang_transcriptions_dict and
segments_dest_lang_transcriptions_dict to use strict=True. This change should be
applied wherever segments_timestamps is paired with the source/dest
transcription lists so mismatches raise ValueError instead of silently
truncating.
- Around line 107-117: The dict_skeleton in org_to_jsonl.py has an inconsistent
English placeholder value: set the "en" entry to None instead of the string
"None" so it matches the other language fields and avoids future type-checking
bugs. Update the dict_skeleton initialization in the same area as the
transcription parsing logic, and keep the existing handling in the later checks
for the dict_skeleton entries unchanged unless needed to preserve behavior.
In `@egs/europarl_st/SRT/local/README.md`:
- Line 30: The README’s “See … for detailed documentation” link currently
self-references via the local README, so it does nothing. Update the link in
this README to point to the recipe-level README instead, or remove the reference
entirely; use the existing documentation-link text in this file as the place to
fix it.
- Around line 85-113: The README path examples and directory tree are
inconsistent with the actual recipe layout, since these helpers are under local/
and are called from prepare.sh. Update the documented script paths and usage
examples in README.md to reference the local/ location and the same entry points
used by the recipe, so readers can run org_to_jsonl.py and the other helpers
without missing-file errors.
In `@egs/europarl_st/SRT/local/train_bpe.py`:
- Around line 85-98: Remove the duplicate <2en> entry from user_defined_symbols
in train_bpe.py so the SentencePiece symbol list contains each language tag only
once. Update the list near the user_defined_symbols declaration to keep the
intended 9 language tokens plus the special symbols, and ensure no repeated
symbol remains before the SentencePiece trainer uses it.
- Around line 100-103: Update the misleading comment near unk_id in
train_bpe.py: the current note says unk_id is fixed to 2, but the actual
assignment in the local train_bpe logic sets unk_id from
len(user_defined_symbols), which evaluates to 11 here. Adjust the comment to
match the real value or describe how unk_id is derived, and keep it consistent
with the surrounding handling of user_defined_symbols and any other code that
depends on unk_id.
In `@egs/europarl_st/SRT/prepare.sh`:
- Line 29: The parse_options helper sourcing in prepare.sh is incorrect and can
hide failures by skipping option parsing; update the . shared/parse_options.sh
line to source the correct helper used by the script and remove the trailing ||
true so --stage and --stop-stage parsing cannot be silently bypassed. Use the
prepare.sh entrypoint and the parse_options.sh sourcing call to locate and fix
this.
---
Nitpick comments:
In `@egs/europarl_st/SRT/lcma_srt/attention_decoder.py`:
- Around line 250-256: The loop in the decoder layer stack uses enumerate in
`attention_decoder.py` but never references `i`, so update the iteration over
`self.layers` in the decoder forward path to traverse the layers directly
without capturing the unused index. Keep the call to each `mod` unchanged and
remove only the unnecessary loop variable so the code satisfies the linter.
In `@egs/europarl_st/SRT/lcma_srt/datamodule.py`:
- Around line 374-465: The validation dataloader logic is duplicated, and the
active method name valid_dataloaders_gpt41 is a placeholder that should be
cleaned up. Keep the distributed-aware implementation in
valid_dataloaders_gpt41, rename it to the intended public name valid_dataloaders
so train.py continues to call the right method, and remove the unused older
valid_dataloaders implementation to avoid dead code and confusion.
- Around line 305-310: Replace the eval(self.args.input_strategy)() usage in the
datamodule setup with a safe dispatch map from allowed strategy names to the
corresponding input strategy constructors, then instantiate the selected one in
K2SpeechRecognitionDataset. Apply the same change in both places where this
pattern appears so the input_strategy resolution is centralized, explicit, and
no longer uses eval().
In `@egs/europarl_st/SRT/lcma_srt/decode.py`:
- Around line 1521-1532: ST WER results are not being saved because the
`st_save_wer_results` path in `decode.py` is commented out. Update the scoring
flow in the block that already calls `asr_save_wer_results` so
`st_save_wer_results` is invoked for ST decoding when appropriate, using
`results_dict_st` and the existing `params.ast_use_asr_data` condition or the
correct gating for ST scoring. Ensure the `st_save_wer_results` function is
actually reached so ST results are recorded.
In `@egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2m.sh`:
- Around line 24-97: The TEST_CUTS_PATHS array is duplicated across decode
launcher scripts, so update decode_hent_srt_m2m.sh to source the shared list
instead of hardcoding it here. Move the language-pair manifest paths into a
common script or generated helper used by decode_lcma_srt.sh and the other
stage2 decode entry points, and keep the existing TEST_CUTS_PATHS symbol as the
single source of truth to prevent drift.
In `@egs/europarl_st/SRT/lcma_srt/moe_adapter.py`:
- Line 52: The `MoEAdapter` dimension calculations contain a redundant outer
`int()` around `round()`, so update the hidden-size expressions in `MoEAdapter`
to use `round(...)` directly and keep the `max(1, ...)` guard intact. Apply the
same cleanup to both occurrences in the adapter initialization logic so the
`d_hidden` and related size computation remain functionally identical but
simpler.
In `@egs/europarl_st/SRT/lcma_srt/train.py`:
- Line 1848: Remove the dead parameter-aliasing loop in run; the setattr calls
in the name iteration are self-assignments and do nothing. Update run (and any
related params setup around the ASR/ST argument handling) to rely on argparse’s
existing underscored attributes instead of creating aliases, and delete the
redundant block entirely.
- Around line 2001-2007: The fallback in the Europarl-ST manifest loading logic
still uses LibriSpeech helper paths, which can trigger a confusing
FileNotFoundError when train.py is run without explicit cut paths. Update the
train/validation setup around the train_cuts and valid_cuts selection logic to
require args.train_cuts_paths and args.valid_cuts_paths for this recipe, or
raise a clear recipe-specific error before calling load_cuts_lazy or any
librispeech.*_cuts helper. Keep the fix aligned with the existing parameter
handling in the training entrypoint so the failure is immediate and informative.
In `@egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_s_bias.sh`:
- Around line 1-135: The stage launcher script duplicates a large shared setup
with the other stage1/stage2 scripts, including the log() helper, environment
exports, architecture defaults, and most of the torchrun arguments. Extract the
common configuration into a shared sourced script (for example a common.sh that
defines log(), env vars, and shared training flags), then keep this script
focused on its stage-specific differences such as data paths, MoE/task settings,
and resume options.
In `@egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc.sh`:
- Around line 1-135: The stage1 launcher duplicates a large shared configuration
block, making flag and path updates easy to miss across variants. Refactor the
common setup in cr_ctc.sh around the shared variable declarations and the
torchrun invocation into a sourced/shared config script, then keep only the
MoE-specific overrides in each launcher. Preserve the existing per-variant
differences by parameterizing the shared values such as ASR_BPE_MODEL,
TRAIN_CUTS_PATHS, encoder dimensions, and language lists, and have cr_ctc.sh
consume that shared config instead of hardcoding everything locally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a70ab0e-ee06-4980-b603-ea8d3859e948
⛔ Files ignored due to path filters (1)
egs/europarl_st/SRT/lcma_srt/LCMA-SRT.pngis excluded by!**/*.png
📒 Files selected for processing (45)
egs/europarl_st/SRT/LICENSEegs/europarl_st/SRT/README.mdegs/europarl_st/SRT/RESULTS.mdegs/europarl_st/SRT/lcma_srt/README.mdegs/europarl_st/SRT/lcma_srt/attention_decoder.pyegs/europarl_st/SRT/lcma_srt/beam_search.pyegs/europarl_st/SRT/lcma_srt/datamodule.pyegs/europarl_st/SRT/lcma_srt/decode.pyegs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc.shegs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_moe.shegs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_s_bias.shegs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_sc_moe.shegs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2m.shegs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2o.shegs/europarl_st/SRT/lcma_srt/decode/stage2/decode_lcma_srt.shegs/europarl_st/SRT/lcma_srt/decoder.pyegs/europarl_st/SRT/lcma_srt/encoder_interface.pyegs/europarl_st/SRT/lcma_srt/joiner.pyegs/europarl_st/SRT/lcma_srt/label_smoothing.pyegs/europarl_st/SRT/lcma_srt/model.pyegs/europarl_st/SRT/lcma_srt/moe_adapter.pyegs/europarl_st/SRT/lcma_srt/optim.pyegs/europarl_st/SRT/lcma_srt/scaling.pyegs/europarl_st/SRT/lcma_srt/subsampling.pyegs/europarl_st/SRT/lcma_srt/train.pyegs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc.shegs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_moe.shegs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_s_bias.shegs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_sc_moe.shegs/europarl_st/SRT/lcma_srt/train/stage2/hent_srt_m2m.shegs/europarl_st/SRT/lcma_srt/train/stage2/hent_srt_m2o.shegs/europarl_st/SRT/lcma_srt/train/stage2/lcma_srt.shegs/europarl_st/SRT/lcma_srt/zipformer.pyegs/europarl_st/SRT/local/README.mdegs/europarl_st/SRT/local/check_manifests.pyegs/europarl_st/SRT/local/filter_cuts_texts.pyegs/europarl_st/SRT/local/normalize_jsonl_with_whisper.pyegs/europarl_st/SRT/local/normalize_texts.pyegs/europarl_st/SRT/local/org_to_jsonl.pyegs/europarl_st/SRT/local/texts_to_cuts.pyegs/europarl_st/SRT/local/train_bpe.pyegs/europarl_st/SRT/local/utils/__init__.pyegs/europarl_st/SRT/local/utils/audio_utils.pyegs/europarl_st/SRT/local/utils/dataset_parameters.pyegs/europarl_st/SRT/prepare.sh
| ▼ | ||
| ┌────────────────────────┐ | ||
| │ filter_cuts_texts.py │ Remove entries with empty text/st_text | ||
| └────────┬───────────────┘ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the self-link.
../local/README.md points back to this file, so the “See … for detailed documentation” link is a no-op. Point it at the recipe-level README or remove it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@egs/europarl_st/SRT/local/README.md` at line 30, The README’s “See … for
detailed documentation” link currently self-references via the local README, so
it does nothing. Update the link in this README to point to the recipe-level
README instead, or remove the reference entirely; use the existing
documentation-link text in this file as the place to fix it.
Collaborator
|
Please ignore the failed CI tests. |
Contributor
Author
Thanks for reviewing. Looking forward to your feedback. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
egs/europarl_st/SRT/lcma_srt/decode.py (1)
1066-1078: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the decoding-method assertion to fail fast.
decode_one_batchnow supports onlymodified_beam_search(every other method raisesNotImplementedErrorat Lines 582-585). This assertion still permits 10 methods, so a run with e.g.--decoding-method greedy_searchpasses validation here, then loads/averages the model, builds the dataloader, and only fails deep insidedecode_one_batch. Restrict the accepted set to the methods actually implemented so the failure is immediate and clear.♻️ Proposed change
- assert params.decoding_method in ( - "greedy_search", - "beam_search", - "fast_beam_search", - "fast_beam_search_nbest", - "fast_beam_search_nbest_LG", - "fast_beam_search_nbest_oracle", - "modified_beam_search", - "modified_beam_search_LODR", - "modified_beam_search_lm_shallow_fusion", - "modified_beam_search_lm_rescore", - "modified_beam_search_lm_rescore_LODR", - ) + assert params.decoding_method == "modified_beam_search", ( + f"Unsupported decoding method '{params.decoding_method}'. " + "This dual ASR/ST decoder only supports 'modified_beam_search'." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egs/europarl_st/SRT/lcma_srt/decode.py` around lines 1066 - 1078, Restrict the decoding-method assertion in decode_one_batch to accept only "modified_beam_search", removing all unsupported methods from the permitted tuple so invalid configurations fail immediately during validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@egs/europarl_st/SRT/lcma_srt/decode.py`:
- Around line 1066-1078: Restrict the decoding-method assertion in
decode_one_batch to accept only "modified_beam_search", removing all unsupported
methods from the permitted tuple so invalid configurations fail immediately
during validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c0b08a7-0f51-4010-9681-22846cb3320d
📒 Files selected for processing (1)
egs/europarl_st/SRT/lcma_srt/decode.py
…path, duplicate symbol, parse_options
Collaborator
|
https://osf.io/request-access/rnuhv 
The checkpoints currently require permission to access. Since both the code and the dataset are open-sourced, would it be possible to make the checkpoints publicly available as well? We recommend hosting the checkpoints on platforms such as ModelScope or Hugging Face. All recipes in icefall provide publicly accessible checkpoints for reproducibility. |
Contributor
Author
Thanks for pointing this out. I’ll make the model checkpoints publicly available on Hugging Face later today and update the repository with the link once the upload is complete. |
csukuangfj
reviewed
Jul 10, 2026
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a complete Europarl-ST SRT recipe (LCMA-SRT) including preprocessing, manifest validation, BPE training utilities, and stage-based train/decode scripts for multilingual ASR+ST.
Changes:
- Added a multi-stage data preparation pipeline (audio segmentation → text normalization → CutSet+FBANK generation → filtering → validation → BPE).
- Introduced LCMA-SRT model components (MoE adapters, subsampling, decoder/joiner, attention decoder) plus runnable stage1/stage2 train & decode scripts.
- Added recipe documentation/results and Apache 2.0 license files under the new Europarl-ST SRT recipe directory.
Reviewed changes
Copilot reviewed 46 out of 48 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| egs/europarl_st/SRT/shared | Adds shared-script linkage for option parsing/utilities. |
| egs/europarl_st/SRT/prepare.sh | End-to-end dataset preparation driver with staged execution. |
| egs/europarl_st/SRT/local/utils/dataset_parameters.py | Centralizes preprocessing constants (e.g., sample rate). |
| egs/europarl_st/SRT/local/utils/audio_utils.py | FFmpeg-based audio segment conversion helpers. |
| egs/europarl_st/SRT/local/utils/init.py | Exposes local preprocessing utilities as a package. |
| egs/europarl_st/SRT/local/train_bpe.py | SentencePiece BPE training + tokens.txt generation. |
| egs/europarl_st/SRT/local/texts_to_cuts.py | Builds Lhotse manifests and computes/stores FBANK features with caching. |
| egs/europarl_st/SRT/local/org_to_jsonl.py | Converts raw Europarl-ST structure into per-lang-pair JSONL and FLAC segments. |
| egs/europarl_st/SRT/local/normalize_texts.py | Whisper-style normalization for per-lang-pair JSONL text fields. |
| egs/europarl_st/SRT/local/normalize_jsonl_with_whisper.py | Whisper normalization for CutSet-format JSONL(.gz) files. |
| egs/europarl_st/SRT/local/filter_cuts_texts.py | Filters manifests by removing empty text / st_text entries. |
| egs/europarl_st/SRT/local/check_manifests.py | Validates manifests in parallel (optionally reading audio/features). |
| egs/europarl_st/SRT/local/README.md | Documents the preprocessing pipeline and outputs. |
| egs/europarl_st/SRT/lcma_srt/train/stage2/lcma_srt.sh | Stage2 joint ASR+ST training launcher for LCMA-SRT. |
| egs/europarl_st/SRT/lcma_srt/train/stage2/hent_srt_m2o.sh | Stage2 baseline (m2o) training launcher. |
| egs/europarl_st/SRT/lcma_srt/train/stage2/hent_srt_m2m.sh | Stage2 baseline (m2m) training launcher. |
| egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_sc_moe.sh | Stage1 multilingual ASR pretraining launcher (SRC-MoE). |
| egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_s_bias.sh | Stage1 baseline training launcher (S-bias). |
| egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc_moe.sh | Stage1 baseline training launcher (MoE). |
| egs/europarl_st/SRT/lcma_srt/train/stage1/cr_ctc.sh | Stage1 baseline training launcher (CR-CTC). |
| egs/europarl_st/SRT/lcma_srt/subsampling.py | Adds ConvNeXt-enhanced convolutional subsampling module. |
| egs/europarl_st/SRT/lcma_srt/moe_adapter.py | Implements language/task-conditioned MoE adapters. |
| egs/europarl_st/SRT/lcma_srt/label_smoothing.py | Provides label-smoothing loss utility. |
| egs/europarl_st/SRT/lcma_srt/joiner.py | Joiner module used by transducer decoding/training. |
| egs/europarl_st/SRT/lcma_srt/encoder_interface.py | Base interface for encoder modules. |
| egs/europarl_st/SRT/lcma_srt/decoder.py | Stateless decoder implementation for RNN-T style models. |
| egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_lcma_srt.sh | Stage2 decode driver over multiple language pairs. |
| egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2o.sh | Stage2 baseline decode script (m2o). |
| egs/europarl_st/SRT/lcma_srt/decode/stage2/decode_hent_srt_m2m.sh | Stage2 baseline decode script (m2m). |
| egs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_sc_moe.sh | Stage1 decode script (SRC-MoE). |
| egs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_s_bias.sh | Stage1 baseline decode script (S-bias). |
| egs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc_moe.sh | Stage1 baseline decode script (MoE). |
| egs/europarl_st/SRT/lcma_srt/decode/stage1/decode_cr_ctc.sh | Stage1 baseline decode script (CR-CTC). |
| egs/europarl_st/SRT/lcma_srt/datamodule.py | Adds a data module for constructing Lhotse-backed DataLoaders. |
| egs/europarl_st/SRT/lcma_srt/attention_decoder.py | Adds an attention-decoder implementation for optional use. |
| egs/europarl_st/SRT/lcma_srt/README.md | Documents LCMA-SRT model/usage within the recipe. |
| egs/europarl_st/SRT/README.md | Top-level recipe documentation and key results tables. |
| egs/europarl_st/SRT/LICENSE | Adds Apache 2.0 license text for the recipe directory. |
| README.md | Updates repository-level README (currently with conflicts). |
Comments suppressed due to low confidence (7)
egs/europarl_st/SRT/local/org_to_jsonl.py:1
- These imports are very likely broken:
utilsis located underlocal/utils, but the script appends the recipe root (.../SRT) tosys.path, not.../SRT/local. As a result,from utils...will fail unless there is an unrelatedutils/at the recipe root. Fix by adjustingsys.pathto include thelocaldirectory (or by turninglocal/into a proper package and importing via it).
egs/europarl_st/SRT/prepare.sh:1 - The pipeline paths in
prepare.shassume JSONL texts live under${data_root}/texts, butlocal/org_to_jsonl.pycurrently writes texts to a hard-coded path relative to the script location (not controlled byprepare.sh). This makes Stage 1 (--src-dir ${texts_dir}) fail because${texts_dir}may never be created/populated. Consider adding a--texts-dir/--texts-output-dirargument toorg_to_jsonl.pyand pass${texts_dir}fromprepare.shso stages are consistent.
egs/europarl_st/SRT/local/texts_to_cuts.py:1 - Using
audio_path.stemasrecording_idcan collide across splits becauseorg_to_jsonl.pynames files like<lang>_<id>.flacwith per-split counters. E.g.,en_1.flaccan exist in bothtrain/andtest/, producing the samerecording_id. Since the feature cache is keyed byrecording_id, this can attach the wrong cached features to a different audio file. Use a recording_id derived from a path that includes the split (or hash the full path), and key the cache by that stable unique id.
egs/europarl_st/SRT/local/texts_to_cuts.py:1 - Using
audio_path.stemasrecording_idcan collide across splits becauseorg_to_jsonl.pynames files like<lang>_<id>.flacwith per-split counters. E.g.,en_1.flaccan exist in bothtrain/andtest/, producing the samerecording_id. Since the feature cache is keyed byrecording_id, this can attach the wrong cached features to a different audio file. Use a recording_id derived from a path that includes the split (or hash the full path), and key the cache by that stable unique id.
egs/europarl_st/SRT/local/texts_to_cuts.py:1 - Writing the feature cache directly to the final path risks leaving a truncated/corrupted JSON file if the process is interrupted (e.g., preemption, OOM). Consider writing to a temporary file in the same directory and atomically replacing the target file to make cache updates crash-safe.
egs/europarl_st/SRT/local/filter_cuts_texts.py:1 - This implementation buffers all kept entries in memory (
filtered_lines) before writing. For large CutSet manifests, this can become a significant memory spike. Prefer streaming output: open the destination file once and write each kept line as you process it.
egs/europarl_st/SRT/shared:1 - This looks like it is intended to be a symlink to
../../../icefall/shared/, but it is committed as a regular file containing the path text. If other scripts expectshared/parse_options.shetc. to resolve via a symlink, this will break at runtime. Consider committing it as an actual symlink (git mode 120000) or replacing usages to reference the real shared directory explicitly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+305
to
+310
| train = K2SpeechRecognitionDataset( | ||
| input_strategy=eval(self.args.input_strategy)(), | ||
| cut_transforms=transforms, | ||
| input_transforms=input_transforms, | ||
| return_cuts=self.args.return_cuts, | ||
| ) |
| fix_random_seed(self.seed + worker_id) | ||
|
|
||
|
|
||
| class LibriSpeechAsrDataModule: |
|
|
||
| return train_dl | ||
|
|
||
| def valid_dataloaders_gpt41(self, cuts_valid: CutSet) -> DataLoader[Any]: |
|
|
||
| return valid_dl | ||
|
|
||
| def valid_dataloaders(self, cuts_valid: CutSet) -> DataLoader: |
Comment on lines
+77
to
+85
| if context_size > 1: | ||
| self.conv = nn.Conv1d( | ||
| in_channels=decoder_dim, | ||
| out_channels=decoder_dim, | ||
| kernel_size=context_size, | ||
| padding=0, | ||
| groups=decoder_dim // 4, # group size == 4 | ||
| bias=False, | ||
| ) |
| set -e | ||
| export DISABLE_VERSION_CHECK=1 | ||
|
|
||
| echo "=== Training script started on $(hostname) at $(date) ===" |
csukuangfj
reviewed
Jul 10, 2026
csukuangfj
left a comment
csukuangfj
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! Looks good to me.
Left only some minor comments about typo fixes.
I will try to add CI to test the checkpoints after merging it.
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
…s.sh Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
…e.sh Co-authored-by: Fangjun Kuang <csukuangfj@gmail.com>
csukuangfj
approved these changes
Jul 10, 2026
csukuangfj
left a comment
csukuangfj
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for your contribution!
Contributor
Author
Thank you for reviewing the changes and pointing out the spelling error. I have addressed it. |
Contributor
Author
Thank you for your review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Structure
egs/europarl_st/SRT/
├── prepare.sh # Data preparation
├── local/ # Preprocessing scripts
├── lcma_srt/ # Model code
│ ├── train/stage1/ # ASR pretraining scripts
│ ├── train/stage2/ # Joint SRT training scripts
│ ├── decode/stage1/ # ASR decoding scripts
│ └── decode/stage2/ # Joint decoding scripts
├── README.md
└── RESULTS.md
Reference
Summary by CodeRabbit
New Features
Documentation
Chores