-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
@@ -7,8 +7,6 @@ | |||
import sys | |||
from typing import Any, Mapping, Optional, Union, cast | |||
|
|||
import openai |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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!
This fixes a small typo in `oaieval.py` where `additional_completion_args` was `additonal_completion_args`. Running `pre-commit` also removed an unused import.
This fixes a small typo in `oaieval.py` where `additional_completion_args` was `additonal_completion_args`. Running `pre-commit` also removed an unused import.
This fixes a small typo in
oaieval.py
whereadditional_completion_args
wasadditonal_completion_args
. Runningpre-commit
also removed an unused import.