-
Notifications
You must be signed in to change notification settings - Fork 793
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
feat: Add task_id to request inference context #5059
feat: Add task_id to request inference context #5059
Conversation
One question, why setting it in response context instead of request, which seems a more proper place to store this information. The response context here is mainly for modifying the properties of the response to be returned. |
@frostming thats a good point and when I saw that the response context is mainly for modifying the properties of the response to be returned I also questioned that. My initial thought was that when I call the submit_task endpoint, then I get the task_id in the response. And from the documentation I understood that the context allows you to access information about the incoming request and it's response. And since the task_id came with the response to my request, I naturally first looked into the response context in my inference function, but didn't find it there and therefore thought about adding it there. I can edit it to add the task_id to request.state which based on the starlette documentation is there to add additional information, would that be better? |
Also Another point I didn't mention is the whole task implementation is local only, it works when you serve the bento without your own infras. And for bento cloud we have another implementation for tasks. |
Well I guess then the actual api function which gets the context is called after the response was sent.
I guess it would still be beneficial to cover this aspect for local implementations. At least I don't see any drawbacks in it. |
Previously, task_id was set in the response context, but after further consideration, it made more sense to move it to the request context. Since task_id is a fixed value and the response context is for modifications, placing it in the request context better reflects its purpose and usage.
What does this PR address?
feat: Add
task_id
tracking to request inference contextDescription
This PR introduces
task_id
tracking inInference context request state
, makingtask_id
accessible within API functions with the task decorator by accessingctx.request.state.task_id
. Based on the starlette documentation the request.state attribute is there to add additional information. This provides access totask_id
across task functions, setting up support for features like state management and task control (e.g., pausing/resuming #5058).Key changes:
task_id
toctx.request.state
, available during task execution.task_id
directly, improving task observability.Edit:
Initially I put the task_id in the response context, but after the comment from @frostming I modified it to be contained within the request context since the response context is mainly for modifying the properties of the response to be returned and task_id is a fixed value and therefore fits better in the request inference context.
Before submitting:
pre-commit run -a
script has passed?