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

[PROPOSAL] Allow the client to pass a callback that receives arguments Instructor sends to LLM providers #911

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

voberoi
Copy link
Contributor

@voberoi voberoi commented Aug 6, 2024

Instructor doesn't expose an easy way to see or use the raw arguments that Instructor finally sends over to a language model: #907, #767, #888 (comment))

This PR is a sketch of an idea that solves this problem in a way that:

  • Allows instructor clients to receive these arguments and do whatever they want with them, every time instructor calls out to an language model provider (including every retry).
  • Doesn't require first-class integrations between a third party logging service and an underlying client for logging.

It does this by allowing users to pass in a callback, like so:

def log_llm_args(args: list[Any], kwargs: dict[str, Any]):
    logger.info(f"LLM args: {args}")
    logger.info(f"LLM kwargs: {kwargs}")


def main():
    client = instructor.from_anthropic(
        Anthropic(api_key=os.getenv("ANTHROPIC_API_KEY"))
    )

    user_info = client.chat.completions.create(
        model="claude-3-haiku-20240307",
        response_model=UserInfo,
        messages=[
            {
                "role": "system",
                "content": "You are an expert at extracting information about users from sentences.",
            },
            {"role": "user", "content": "John Doe is 30 years old."},
        ],
        max_tokens=4096,
        max_retries=3,
        provider_args_callback=log_llm_args, # This will get called every time `instructor` calls out to an LLM provider with the same args/kwargs.
    )

    print(user_info.name)
    print(user_info.age)


if __name__ == "__main__":
    main()

The code below is a working implementation for Anthropic's synchronous client, that applies to create and create_with_completion.

This is more-or-less what it'd look like for other providers if this is something you want to do.

An extension we could add is to provide an attempt_num and intermediate completions received before retries, which might be helpful for some use cases.

I'm personally keen to have this kind of functionality in instructor, some way or another (vs. just logging)

If the strategy below works for you, I can submit a PR that works with sync/async and the major providers (OpenAI/Anthropic/Google).

If not, would love to know either:

a) This is not a problem you care to solve.
b) You also want to solve it, but differently.

Thanks!


🚀 This description was created by Ellipsis for commit ed5aeb9

Summary:

Introduced provider_args_callback to log or handle arguments passed to language model providers in instructor.

Key points:

  • Added provider_args_callback parameter to create, create_with_completion, and other relevant methods in instructor/client.py.
  • Updated patch function in instructor/patch.py to handle provider_args_callback.
  • Modified retry_sync and retry_async functions in instructor/retry.py to call provider_args_callback with request arguments and keyword arguments.
  • Allows clients to pass a callback to log or handle arguments sent to language model providers.

Generated with ❤️ by ellipsis.dev

@voberoi voberoi changed the title [Proposal] Allow the client to pass a callback that receives arguments Instructor sends to the client [Proposal] Allow the client to pass a callback that receives arguments Instructor sends to LLM providers Aug 6, 2024
@voberoi voberoi changed the title [Proposal] Allow the client to pass a callback that receives arguments Instructor sends to LLM providers [PROPOSAL] Allow the client to pass a callback that receives arguments Instructor sends to LLM providers Aug 6, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to ed5aeb9 in 1 minute and 31 seconds

More details
  • Looked at 238 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_Mu7WEfmRYd5TFu8u


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ivanleomk
Copy link
Collaborator

Hmm personally I think this could be solved by using something like logfire or langsmith which would help capture and log the information a lot better. Have you tried those options?

@voberoi
Copy link
Contributor Author

voberoi commented Aug 7, 2024

I use third-party logging tools like those. I don't use those ones in particular, but I'm familiar with them.

I don't have issues with such tools. They're important in production use cases.

However, there are scenarios where:

  1. I want to access these messages to log them locally or save them elsewhere, without using a third party logging tool.
  2. I want more visibility into retries.
  3. The logging provider I am using has not instrumented the underlying client.

I also don't think the default mode to access whatever instructor sends to a provider should be to send data to a third-party provider that happens to have instrumented instructor or the underlying client.

@jxnl
Copy link
Collaborator

jxnl commented Aug 7, 2024

What are your thoughts on attaching the callbacks to the constructor of the client rather than the create call?

from_openai(client, callbacks=[])

@voberoi
Copy link
Contributor Author

voberoi commented Aug 7, 2024

That works, but something to consider is that clients will frequently want to tie these callbacks to a specific invocation, like so:

def get_logging_callback(some_id: str):
    def log_llm_args(args: list[Any], kwargs: dict[str, Any]):
        logger.info(f"Calling LLM provider for invocation {some_id}")
        logger.info(f"LLM args: {args}")
        logger.info(f"LLM kwargs: {kwargs}")
    return log_llm_args


def invoke_llm_for_task(task_id: str):
    client = instructor.from_anthropic(
        Anthropic(api_key=os.getenv("ANTHROPIC_API_KEY"))
    )

    user_info = client.chat.completions.create(
        model="claude-3-haiku-20240307",
        response_model=UserInfo,
        messages=[
            {
                "role": "system",
                "content": "You are an expert at extracting information about users from sentences.",
            },
            {"role": "user", "content": "John Doe is 30 years old."},
        ],
        max_tokens=4096,
        max_retries=3,
        provider_args_callback=get_logging_callback(task_id)
    )

I always instantiate the client before invocations or a span of them in my code base, so attaching the callbacks to the constructor is easy.

If instructor users are passing instantiated instructor clients around, this change might be more challenging for them to take advantage of.

Or, if you decide to make clients stateful such that passing a previously-instantiated client around is the recommended way to use the library, that might make things challenging later.

I don't see why you'd do that. Still, I think attaching the callbacks to the create* calls themselves might make more sense.

Your call.

Some other considerations unrelated to the above:

  • Should these callbacks be called before or after an invocation? I think before, since clients will know what instructor sent over to LLM providers whether or not the call failed (they can catch exceptions).
  • Should the callbacks receive an attempt_num? I don't see why not, it's useful information and the only way to figure it out otherwise is by looking through messages (which will be formatted differently for every provider).
  • Should we add some DEBUG logging at the same point the callback is called? This is a common request (see the issues/comments I link to above) and doing this out of the box might be a nice QoL addition. But we don't have to, and can instead leave it to users to do this themselves with this callback (with an example in the docs).

@voberoi
Copy link
Contributor Author

voberoi commented Aug 15, 2024

Hi! Bumping to see if this is something you'd want to do.

@jxnl
Copy link
Collaborator

jxnl commented Sep 23, 2024

sorry been out.

i want to do this but almost in the client, vs the create call. what are your thoughts on this?

@jxnl
Copy link
Collaborator

jxnl commented Oct 4, 2024

we're going to clean this up a bit with a refactor that will make it easier

@voberoi
Copy link
Contributor Author

voberoi commented Oct 9, 2024

Hey! Sorry just saw this.

Yeah I'm fine with that strategy but also, keep me posted on the refactor and I can do it after.

Thank you!

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.

3 participants