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

add option to pass 'api_key' to gen_answers, judge_answers #128

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Sep 11, 2024

I notice the api_key is hard-coded which is preventing from using an external judge server. This adds an optional api_key to provide an openai_client in gen_answers and judge_answers. Note, candidate-server & judge-server may have unique tokens so the env var OPENAI_API_KEY should not be used.

Copy link
Member

@danmcp danmcp left a comment

Choose a reason for hiding this comment

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

+1 to moving to judge and generate in mt_bench. Needs to be an option where ever server_url is passed.

@mergify mergify bot added testing Relates to testing ci-failure labels Sep 12, 2024
@sallyom sallyom changed the title look for OPENAI_API_KEY before setting to NO_API_KEY add option to pass 'api_key' to gen_answers, judge_answers Sep 12, 2024
@mergify mergify bot added ci-failure and removed ci-failure labels Sep 12, 2024
@nathan-weinberg
Copy link
Member

@sallyom thanks for the contribution! some CI failures here - you can run the checks locally with make verify - ping me if you have any issues there 😄

src/instructlab/eval/mt_bench.py Outdated Show resolved Hide resolved
src/instructlab/eval/mt_bench_answers.py Outdated Show resolved Hide resolved
src/instructlab/eval/mt_bench_judgment.py Outdated Show resolved Hide resolved
tests/test_branch_judge_answers.py Outdated Show resolved Hide resolved
src/instructlab/eval/mt_bench_common.py Outdated Show resolved Hide resolved
src/instructlab/eval/mt_bench.py Outdated Show resolved Hide resolved
Copy link
Member

@danmcp danmcp left a comment

Choose a reason for hiding this comment

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

Looks good except my question around whether the test changes are helpful and the commits need to be squashed and/or made more descriptive.

tests/test_branch_gen_answers.py Outdated Show resolved Hide resolved
`api_key` is optional and this PR remains backwards compatible.
This allows for externally served models that require authentication.
A helper function is added in mt_bench_common for creating the
openai_client necessary for model requests.

Signed-off-by: sallyom <somalley@redhat.com>
@sallyom
Copy link
Contributor Author

sallyom commented Sep 13, 2024

thanks for your review @danmcp, I really appreciate your help.

@danmcp
Copy link
Member

danmcp commented Sep 13, 2024

thanks for your review @danmcp, I really appreciate your help.

Thanks for the commit and working through the minutia!

@mergify mergify bot added the one-approval label Sep 13, 2024
Copy link
Member

@nathan-weinberg nathan-weinberg left a comment

Choose a reason for hiding this comment

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

Appreciate the contribution @sallyom!

@mergify mergify bot removed the one-approval label Sep 13, 2024
@mergify mergify bot merged commit 83f9d95 into instructlab:main Sep 13, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants