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

Replace num_beams with generations_per_sample #971

Closed
wants to merge 7 commits into from

Conversation

eitanturok
Copy link
Contributor

In composer's build_icl_dataloader function (here), we specify the number of generations per sample with the variable generations_per_sample.

But when we call build_icl_dataloader in llm-foundry here, we specified the number of generations per sample with the variable num_beams. This was confusing because

  1. we use different variable names
  2. the variable num_beams is also used as a parameter for beam search in inference.

To resolve this, I replaced all instances of num_beams with generations_per_sample in llm-foundry. If generations_per_sample is not specified in the config and num_beams is specified, we will use the value set by num_beams and raise a warning that num_beams is depreciated.

Cheers!

@eitanturok
Copy link
Contributor Author

Overall, these changes are so that we use the same variable names in both llmfoundry and composer.

Right now, the PR Make CodeEval respect device_eval_batch_size uses the variable generations_per_sample to specify the number of generations per sample and so we change num_beams, which is how we specify the number of generations per sample in llmfoundry, to generations_per_sample.

@josejg
Copy link
Contributor

josejg commented Feb 14, 2024

I believe this PR is redundant with the accompanying foundry PR of the composer you linked: #956

I didn't raise a DeprecationWarning because I do think specifying num_beams was a bug and misnomer, and thus should not be supported any longer as it's a footgun.

@dakinggg
Copy link
Collaborator

I'm going to close this PR, it is being covered in #956 . Let me know if you disagree.

@dakinggg dakinggg closed this Feb 15, 2024
@eitanturok
Copy link
Contributor Author

@josejg -- that makes sense, specifying num_beams is a bug and should not have a DeprecationWarning.

@dakinggg -- no worries, these changes can be implemented in that other PR.

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.

4 participants