Skip to content
Discussion options

You must be logged in to vote

Thanks @boyangsvl — agreed on all three points, and they reframe #6222.

On Alternative #1: you're right that it's not just suboptimal, it's unsound. Since a before_model_callback can return a new LlmRequest (not only mutate it in place), stashing the reference in before_model_callback and serializing it later can capture the wrong object. That also reinforces why the fix has to read whatever object is actually passed to generate_content_async, rather than a reference captured earlier.

On the hook being read-only: makes sense. I'll change the proposed signature from -> Optional[LlmResponse] to a pure observer (-> None, no ability to short-circuit or mutate). That makes my on_model_request …

Replies: 2 comments 7 replies

Comment options

You must be logged in to vote
0 replies
Comment options

You must be logged in to vote
7 replies
@haiyuan-eng-google
Comment options

@haiyuan-eng-google
Comment options

@boyangsvl
Comment options

@haiyuan-eng-google
Comment options

Answer selected by llalitkumarrr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Category
Q&A
Labels
None yet
5 participants