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.

Sign up for GitHub

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

Jump to bottom

Fix small typo in oaieval run function #1438

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Fix small typo in oaieval run function #1438

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions evals/cli/oaieval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!


import evals
import evals.api
import evals.base
Expand Down Expand Up @@ -137,11 +135,11 @@ def run(args: OaiEvalArguments, registry: Optional[Registry] = None) -> str:

# If the user provided an argument to --completion_args, parse it into a dict here, to be passed to the completion_fn creation **kwargs
completion_args = args.completion_args.split(",")
additonal_completion_args = {k: v for k, v in (kv.split("=") for kv in completion_args if kv)}
additional_completion_args = {k: v for k, v in (kv.split("=") for kv in completion_args if kv)}

completion_fns = args.completion_fn.split(",")
completion_fn_instances = [
registry.make_completion_fn(url, **additonal_completion_args) for url in completion_fns
registry.make_completion_fn(url, **additional_completion_args) for url in completion_fns
]

run_config = {
Expand Down
Loading