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

feat: Add task_id to request inference context #5059

Merged

Conversation

Leon-Sander
Copy link
Contributor

@Leon-Sander Leon-Sander commented Nov 3, 2024

What does this PR address?

feat: Add task_id tracking to request inference context

Description

This PR introduces task_id tracking in Inference context request state, making task_id accessible within API functions with the task decorator by accessing ctx.request.state.task_id. Based on the starlette documentation the request.state attribute is there to add additional information. This provides access to task_id across task functions, setting up support for features like state management and task control (e.g., pausing/resuming #5058).

Key changes:

  • Adds task_id to ctx.request.state, available during task execution.
  • Enables task functions to retrieve 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:

@Leon-Sander Leon-Sander requested a review from a team as a code owner November 3, 2024 00:15
@Leon-Sander Leon-Sander requested review from jianshen92 and removed request for a team November 3, 2024 00:15
@frostming
Copy link
Contributor

frostming commented Nov 3, 2024

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.

@Leon-Sander
Copy link
Contributor Author

Leon-Sander commented Nov 3, 2024

@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?

@frostming
Copy link
Contributor

frostming commented Nov 3, 2024

Also _run_task is called after the response is sent, so as background task, so I wonder how you can access this property.

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.

@Leon-Sander
Copy link
Contributor Author

Leon-Sander commented Nov 3, 2024

Also _run_task is called after the response is sent, so as background task, so I wonder how you can access this property.

Well I guess then the actual api function which gets the context is called after the response was sent.
Within _run_task the function api_endpoint_wrapper is called which calls the function api_endpoint. Within those two functions the other context variables are set and I guess the actual api function is called. So by accessing it in _run_task I am accessing it even earlier than the main code as far as I understood it.

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.

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.
@Leon-Sander Leon-Sander changed the title feat: Add task_id to ResponseContext and set it in _run_task under server/app feat: Add task_id to request inference context and set it in _run_task under server/app Nov 3, 2024
@aarnphm aarnphm changed the title feat: Add task_id to request inference context and set it in _run_task under server/app feat: Add task_id to request inference context Nov 5, 2024
@aarnphm aarnphm merged commit 6742572 into bentoml:main Nov 5, 2024
51 checks passed
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