Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configs for wav2vec experiments #259

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AndreasPlt
Copy link
Contributor

Previously, all wav2vec experiments were located in a single file. After adding more and more experiments, this got more chaotic and less modular, hindering flexible development and experimentation. I therefore split the previous config_phoneme_pretrain_finetune.py into different files for each wav2vec modification. Furthermore, I added configs for the experiments with hard negatives and positive samples.

Note: I need to test-run the new configs to see if I transferred everything correctly. Until then, this PR remains as a draft, but please feel welcome to give feedback on the current config structure :)

@vieting vieting self-assigned this Jan 7, 2025
Copy link
Contributor

@vieting vieting left a comment

Choose a reason for hiding this comment

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

Looks mostly good, some comments.

Comment on lines 76 to 88
model_conf_w2v = base_model_conf.copy()
model_conf_w2v["w2v_path"] = phon_boundary_pretrain_job.out_models[CHECKPOINT].model
model_conf_w2v["mask_strategy"] = "phoneme"
model_conf_w2v["mask_length"] = 1
eow_phon_ls100_ctc_base(
model_conf_w2v=model_conf_w2v,
train_name_suffix=os.path.join(
"w2v_phoneme_boundary_masking",
"1_phoneme_spec",
f"checkpoint_{CHECKPOINT}"
),
fairseq_root=fairseq_root,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same as above?

Copy link
Contributor Author

@AndreasPlt AndreasPlt Jan 28, 2025

Choose a reason for hiding this comment

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

Yes, this is just for adding all relevant jobs under the new alias 1_phoneme_spec. Since the amount of jobs that is saved under the train_name_suffix, I thought this approach is easier than calling job.add_alias(...) on all relevant jobs. Feel free to suggest any other approach to accomplish this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

But do we need the old alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base model represents four different models in the ablations:

  1. Testing the w2v modification itself (i.e. just the normal "w2v_phoneme_boundary_masking" alias prefix)
  2. Testing masking strategies in finetuning (there, for negatives_other, the base model is the same for the ablation with random masking strategy; for negatives_other_boundary_masking and boundary_masking, the baseline model is the same as the ablation with phoneme masking strategy)
  3. Testing phoneme mask length in finetuning (only for negatives_other_boundary_masking and boundary_masking, where it is the same as the ablation with mask_len = 1)
  4. Testing masking probability in finetuning (base model is the same as ablation with mask probability 0.65)

I wanted to make this structure clear in the alias folder as well - otherwise, it is not clear from the outside that the base model also represents the "masking probability = 0.65" model. So in that sense, yes, we need the old alias and all other ones as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, then maybe put a comment above the block like "repeat same experiment to obtain additional alias in line with following experiments" and it should be fine.

@AndreasPlt AndreasPlt marked this pull request as ready for review January 28, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants