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

Add callbacks for debugging #46

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

petrus-jvrensburg
Copy link
Contributor

This PR would add two new params to chat_completion, namely inspect_request? and inspect_response? which has the effect of calling IO.inspect on the request, just before sending it to the LLM, or on the response before processing it.

I needed this for debugging, so I think others might find it useful.

@thmsmlr How does this sit with you? An alternative might be to pass in a callback, in stead of setting a flag, and then the user can log / inspect or do whatever they want in the callback. Any thought?

@petrus-jvrensburg
Copy link
Contributor Author

On second thought, I added two callbacks, namely: before_request and after_response. This can be useful for debugging, but also for doing things like keeping track of token usage or API rate limits in the response.

@petrus-jvrensburg petrus-jvrensburg changed the title Add params for debugging Add callbacks for debugging Apr 22, 2024
@rhcarvalho
Copy link

@petrus-jvrensburg did you consider using a custom adapter through http_options (passed to Req)? https://hexdocs.pm/req/Req.Request.html#module-adapter

I didn't try it (yet), but imagine we could use that to intercept requests without requiring changes to instructor (and avoid adding API surface one can regret later).

@petrus-jvrensburg
Copy link
Contributor Author

Thanks @rhcarvalho.

No I haven't considered that, but looking into it now: I still think it's simpler to run some function on the request that doesn't interfere with the library's internals. When using a custom adapter, it looks like the adapter would take over the sending of the request entirely. So you'd break things if the adapter didn't return the response in the way the library anticipates internally. What do you think?

@rhcarvalho
Copy link

I needed this for debugging, so I think others might find it useful.

I still think it's simpler to run some function on the request that doesn't interfere with the library's internals. When using a custom adapter, it looks like the adapter would take over the sending of the request entirely. So you'd break things if the adapter didn't return the response in the way the library anticipates internally. What do you think?

I wanted to offer an existing way to do the debugging, avoiding the need to add more code to this library. I used Req before, outside of Instructor, and being able to influence the pipeline is quite nice -- I used that for the things you mentioned: debugging, logging, tracking API call spend.

I think that the proposed before_request and after_response are more limited and less composable than what Req gives, while still coupled to Req as the functions receive a %Req.Request{} as input.

So if one needs to lean on Req anyway, why not learn and use the Req API, possibly reusing Req.run_finch wrapped in whatever code they want :)

Alternatively, being able to add steps to the request pipeline would have a similar effect, but not require changing the adapter -- just happens that the adapter is the one central piece of the pipeline, but sometimes you want to do things as the very first or very last step too.

@@ -257,7 +253,7 @@ end
#Ecto.Changeset<
action: nil,
changes: %{question: "What is the meaning of life?", answer: "Sex, drugs, and rock'n roll"},
errors: [answer: {"is invalid, Do not say anything objectionable", []}],
errors: [answer: {"is invalid, Contains objectionable content", []}],

Choose a reason for hiding this comment

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

Some changes to this file look unrelated to the rest of the PR. Here the error message is based on line 230 https://github.com/thmsmlr/instructor_ex/pull/46/files#diff-dbcbac73b1730f428122f43c1c48a1614650f5eb3ebd5319ffdbe293ea937a3cR230

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.

2 participants