Skip to content

CI check fixes#2029

Merged
Adel-Moumen merged 9 commits into
speechbrain:developfrom
asumagic:fix-ci-checks
Jun 15, 2023
Merged

CI check fixes#2029
Adel-Moumen merged 9 commits into
speechbrain:developfrom
asumagic:fix-ci-checks

Conversation

@asumagic

@asumagic asumagic commented Jun 14, 2023

Copy link
Copy Markdown
Collaborator

Contribution in a nutshell

#1600 was merged semi-recently, and certain older PRs, which have not been rebased on develop, didn't include these changes at the time the CI checks were executed.

As a result, certain PRs were merged even though they would fail with the checks that develop had, temporally at the same time.

This PR aims to fix the hyperparameter files so that the CI checks run again. After this is merged, other older PRs that contain hyperparameters changes should ideally be rebased to develop/have develop merged into them so that their checks are correct.

Scope

  • CI should pass again

Notes for reviewing (optional)

For the ESC50 recipes, it looks like most unused hyperparameters are leftovers from UrbanSound8k recipes. For the most part, I removed them if they seemed irrelevant, left some commented that seemed like someone would want to tinker with them (namely augmentation-related), or fixed references (e.g. n_mels).

For the WavLM hyperparameter files the unused key seemed irrelevant to the experiment.

Finally, for hf_whisper_encoder, I removed the hparam as @Adel-Moumen asked me to.

I imagine getting this double-checked by the relevant people, namely for #1605, might be good.

Pre-review

  • create a fresh testing environment (install SpeechBrain from cloned repo branch of this PR)
  • use CI locally: pre-commit run -a to check linters; run pytest tests/consistency

Adel-Moumen
Adel-Moumen requested changes Jun 14, 2023
View reviewed changes

@Adel-Moumen Adel-Moumen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

see comments

Comment thread recipes/LibriSpeech/ASR/CTC/hparams/downsampled/train_hf_wavlm_signal_downsampling.yaml
Comment thread recipes/LibriSpeech/ASR/CTC/hparams/train_hf_whisper_encoder.yaml Outdated
asumagic added 2 commits June 15, 2023 09:16
@asumagic
Revert "Fix unused yaml param in wavlm recipes"
029c021 
This reverts commit fbd04db.

The variable is actually used, and there is a bug in the consistency check regexes.
@asumagic
Bypass bug in hparams yaml consistency test
3d61e12
@asumagic

asumagic commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator Author

So there still seems to be a bug where the yaml consistency checks can miss some variables uses in .py files when they span multiple lines. For now I've just worked around the issue by making the sole detected case go on a single line, as fixing the tests doesn't seem trivial. We could look into it further later but maybe not now :)

Adel-Moumen
Adel-Moumen reviewed Jun 15, 2023
View reviewed changes
Comment thread recipes/LibriSpeech/ASR/CTC/hparams/train_hf_whisper_encoder.yaml Outdated
asumagic
asumagic commented Jun 15, 2023
View reviewed changes
Comment thread recipes/LibriSpeech/ASR/CTC/hparams/train_hf_whisper_encoder.yaml Outdated
Adel-Moumen
Adel-Moumen reviewed Jun 15, 2023
View reviewed changes
Comment thread recipes/ESC50/classification/hparams/conv2d_classifier.yaml Outdated
Adel-Moumen
Adel-Moumen reviewed Jun 15, 2023
View reviewed changes
Comment thread recipes/ESC50/interpret/hparams/piq.yaml Outdated
@Adel-Moumen

Adel-Moumen commented Jun 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Hmmm I do prefer to remove comments from yaml if they are not used. Cleaner like that. Could you please run the recipe testing tool on the recipes that have changed please? (just to make sure that it does not break anything...)

@asumagic

asumagic commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator Author

Hmmm I do prefer to remove comments from yaml if they are not used. Cleaner like that. Could you please run the recipe testing tool on the recipes that have changed please? (just to make sure that it does not break anything...)

Will do

@asumagic asumagic self-assigned this Jun 15, 2023
@asumagic

asumagic commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator Author

Removed the commented lines. The recipe tests are running in the mean time.

@Adel-Moumen

Adel-Moumen commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator

Ok great. You just have to run it for the recipes that you modified. (No need to run for the entire toolkit as it might take A LOT of time).

@asumagic

asumagic commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator Author

CI checks failed to a huggingface server error FWIW, might be worth rerunning a bit later.

@asumagic

asumagic commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator Author

With the latest commit included, ESC50 recipe tests pass, and all but 4 LibriSpeech tests pass, all with the following error, which seems irrelevant to these changes:

KeyError: 'The item $rir_rootRIRS_NOISES/pointsource_noises/noise-free-sound-0000.wav requires replacements which were not supplied.'

cc @Adel-Moumen. Should be good?

@asumagic asumagic removed their assignment Jun 15, 2023
Adel-Moumen
Adel-Moumen approved these changes Jun 15, 2023
View reviewed changes

@Adel-Moumen Adel-Moumen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Ty.

@Adel-Moumen Adel-Moumen merged commit 6eb53b8 into speechbrain:develop Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Adel-Moumen Adel-Moumen Adel-Moumen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asumagic @Adel-Moumen