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

Fixed generation args issue affection OpenAI completion model #1458

Merged

Conversation

Am1n3e
Copy link
Contributor

@Am1n3e Am1n3e commented Feb 22, 2024

The task's requests use the generation kwargs as request args when the task type is generate_until as shown below

arguments = (ctx, self.config.generation_kwargs)

Then, in the OpenAI Completion model, the until attribute is poped from the request args:

until = request_args.pop("until", ["<|endoftext|>"])

Since the object (self.config.generation_kwargs) is mutable, once the attribute is poped on the next request it will not be found and default in this case to ["<|endoftext|>"] instead to what every value was in until (provided from the task config file or the --gen_kwargs argument.

This means that only the first request will have the correct until value and any subsequent request will use the default.

@haileyschoelkopf
Copy link
Collaborator

Thanks for the bug report and fix!

Could we also add a copy prior to the popping of attributes from request_args within the OpenAI models, to be on the safe side?

self._max_gen_toks = request_args.pop("max_gen_toks", self.max_gen_toks)

@Am1n3e
Copy link
Contributor Author

Am1n3e commented Feb 22, 2024

@haileyschoelkopf How about just using get in the model? This will avoid doing copies in multiple places.

@Am1n3e
Copy link
Contributor Author

Am1n3e commented Feb 22, 2024

Something like:

            self._max_gen_toks = request_args.get("max_gen_toks", self.max_gen_toks)
            for context, _ in chunk:
                context_enc = self.tok_encode(context)
                inp = context_enc[-(self.max_length - self.max_gen_toks) :]
                inps.append(inp)

            until = request_args.get("until", ["<|endoftext|>"])
            request_args["temperature"] = request_args.get("temperature", 0)

            response = oa_completion(
                client=self.client,
                model=self.model,
                prompt=inps,
                max_tokens=self.max_gen_toks,
                stop=until,
                seed=self.seed,
                **{k: v for k, v in request_args.items() if k not in ["do_sample", "max_gen_toks"]},
            )

@haileyschoelkopf
Copy link
Collaborator

@Am1n3e Sure, that works for me!

@haileyschoelkopf
Copy link
Collaborator

Per @baberabb --test failures are due to the no-copy behavior being relied upon here: https://github.com/Am1n3e/lm-evaluation-harness-ae/blob/5c4e0aa7e4802191c60ec20862ad59d16e702457/tests/models/test_huggingface.py#L25C1-L26C71

changing the order of these two lines in the test should make it safe to do this copy where you've introduced it!

@Am1n3e
Copy link
Contributor Author

Am1n3e commented Feb 22, 2024

@haileyschoelkopf I've addressed the comments.

@haileyschoelkopf
Copy link
Collaborator

Thank you again!

@haileyschoelkopf haileyschoelkopf merged commit 75ac1f4 into EleutherAI:main Feb 22, 2024
7 of 8 checks passed
wx-zhang pushed a commit to wx-zhang/lm-evaluation-harness that referenced this pull request Mar 13, 2024
…erAI#1458)

* Fixed generation args issue affection openai completion model

* Fixed hf unit test; removed pop attributes in OpenAi completion.

* fix format

* fix format

---------

Co-authored-by: Hailey Schoelkopf <65563625+haileyschoelkopf@users.noreply.github.com>
nightingal3 pushed a commit to mycoalchen/lm-evaluation-harness that referenced this pull request May 2, 2024
…erAI#1458)

* Fixed generation args issue affection openai completion model

* Fixed hf unit test; removed pop attributes in OpenAi completion.

* fix format

* fix format

---------

Co-authored-by: Hailey Schoelkopf <65563625+haileyschoelkopf@users.noreply.github.com>
djstrong pushed a commit to speakleash/lm-evaluation-harness that referenced this pull request Aug 2, 2024
…erAI#1458)

* Fixed generation args issue affection openai completion model

* Fixed hf unit test; removed pop attributes in OpenAi completion.

* fix format

* fix format

---------

Co-authored-by: Hailey Schoelkopf <65563625+haileyschoelkopf@users.noreply.github.com>
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