-
Notifications
You must be signed in to change notification settings - Fork 3
Adding a lower-level event and a logger example #382
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
Conversation
jfeser
left a comment
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.
This looks mostly good! Two comments:
llm_requestshould have essentially the same interface asclient.responses.create:
@defop
def llm_request(client: openai.OpenAI, *args, **kwargs) -> Any:
"""Low-level LLM request. Handlers may log/modify requests and delegate via fwd()."""
return client.responses.create(*args, **kwargs)- The notebook needs to be rerun since it has an exception.
|
You don't have to add it in this PR, but I'd be happy to have a llm logging handler that exposes a python logger and logs |
I see, that makes sense, I can make a short attempt, if it is too hard, then I'll revert. |
|
I added the logger that expose Python logger: # 1. Create a logger
logger = logging.getLogger("effectful.llm")
logger.setLevel(logging.INFO)
h = logging.StreamHandler(sys.stdout)
h.setLevel(logging.INFO)
h.setFormatter(logging.Formatter("%(levelname)s %(name)s %(message)s %(payload)s"))
logger.addHandler(h)
# 2. Pass it to the handler
llm_logger = LLMLoggingHandler(logger=logger)
# Avoid cache for demonstration
try:
haiku.cache_clear()
except Exception:
pass
with handler(provider), handler(llm_logger):
_ = haiku("fish3")
_ = limerick("fish4") |
effectful/handlers/llm/providers.py
Outdated
| *, | ||
| logger: logging.Logger | None = None, | ||
| logger_name: str = "effectful.llm", | ||
| level: int = logging.INFO, |
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.
This shouldn't be a configuration parameter. Instead the logger should be exposed as an attribute.
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.
Still outstanding.
effectful/handlers/llm/providers.py
Outdated
| *, | ||
| logger: logging.Logger | None = None, | ||
| logger_name: str = "effectful.llm", | ||
| level: int = logging.INFO, |
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.
Still outstanding.
jfeser
left a comment
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.
Happy to merge once you address outstanding comments.
|
Updated: only have one optional parameter |
Fix #381