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

Fix small typo in oaieval run function #1438

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

inwaves
Copy link
Contributor

@inwaves inwaves commented Dec 20, 2023

This fixes a small typo in oaieval.py where additional_completion_args was additonal_completion_args. Running pre-commit also removed an unused import.

@@ -7,8 +7,6 @@
import sys
from typing import Any, Mapping, Optional, Union, cast

import openai
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we actually need this import? on line 268 we have logging.getLogger("openai").setLevel(logging.WARN). I wonder if importing openai here is needed to init the logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to re-add that import – it's the safer option.

That said, I don't think the import is needed by the logger. Actually, I'm unsure on the purpose of line 268: it's searching for a logger called openai and setting its level to just warnings. There's no logger by that name in any of the files in the repo – and if there were we'd want to change its name so it doesn't shadow the package. So I think it might not be doing anything at all. Maybe the package has a built-in logger we want to piggyback off?

(It looks like it was added in one of the earliest commits by @andrew-openai, which maybe has more context here.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. I looked into how python logging works, and calling getLogger will create a new logger instance if it hasn't been created before, so this should be fine. in openai-python we set the logger level in some setup steps (https://github.com/openai/openai-python/blob/main/src/openai/_utils/_logs.py#L16) but i think since the linter says to remove this it's preferred. thanks for the fix!

@etr2460 etr2460 merged commit 23ae8ab into openai:main Dec 21, 2023
1 check failed
jacobbieker pushed a commit to withmartian/-ARCHIVED--router-evals that referenced this pull request Jan 9, 2024
This fixes a small typo in `oaieval.py` where
`additional_completion_args` was `additonal_completion_args`. Running
`pre-commit` also removed an unused import.
Linmj-Judy pushed a commit to TablewareBox/evals that referenced this pull request Feb 27, 2024
This fixes a small typo in `oaieval.py` where
`additional_completion_args` was `additonal_completion_args`. Running
`pre-commit` also removed an unused import.
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.

None yet

2 participants