-
Notifications
You must be signed in to change notification settings - Fork 8
feat(adapter): add optional update for progress tracking on timer #423
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
base: main
Are you sure you want to change the base?
feat(adapter): add optional update for progress tracking on timer #423
Conversation
| params.progress_tracking_interval_seconds | ||
| ) | ||
| self._update_on_timer_task = None | ||
| if self.progress_tracking_interval_seconds: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question / concern: since this is asyncio inside multithreaded Flask, could you clarify how this task would behave? Is the asyncio loop executed on in the main Flask thread where the object is created, and would that keep blocking? There are suggestions on SO to have an asyncio on a separate thread to avoid issues with asyncio + threading marriage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick review! I’ve verified asyncio.create_task working with fastapi but have not with flask, this is a good callout to do extra testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test on the adapter layer and found that asyncio.create_task errors with no running event loop which tracks with your series of questions about the asyncio loop. Once I reworked the background timer to run as a thread and removed asyncio.sleep, the Flask server seems to running as expected and the timer thread does not block incoming requests. Lmk what you think!
Signed-off-by: Kim Ngo <[email protected]>
Signed-off-by: Kim Ngo <[email protected]>
f0de971 to
fe8a4b9
Compare
|
I think we can merge this since you tested this on this adapter. |
Inference can be slow depending on the model performance, the number of GPUs the model is running on, the context length or tokens to generate or parallelism. For slow inference times, an interval based progress tracking may be a frustrating user experience to wait 30mins or longer for the first progress update.
This PR adds an optional parameter to ProgressTracking adapter to update progress on a timed interval, not enabled by default. ProgressTrackingInterceptor tracks the last status that was posted, and skips sending a request / writes to file if there has not been a change. The timed update is run as a asyncio task that is stopped on
post_eval_hook