fix(cli): handle sync/async serve functions in m serve#784
Open
markstur wants to merge 6 commits intogenerative-computing:mainfrom
Open
fix(cli): handle sync/async serve functions in m serve#784markstur wants to merge 6 commits intogenerative-computing:mainfrom
markstur wants to merge 6 commits intogenerative-computing:mainfrom
Conversation
Contributor
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
markstur
changed the title
Fixes sync/async mismatch in `m serve` by detecting function type and handling appropriately: - Async serve functions are awaited directly - Sync serve functions are wrapped in asyncio.to_thread() to prevent blocking FastAPI's event loop This ensures the server can handle concurrent requests efficiently regardless of whether user-defined serve functions are sync or async. Changes: - cli/serve/app.py: Add asyncio/inspect imports, update make_chat_endpoint() to detect coroutine functions and wrap sync functions in to_thread() - test/cli/test_serve_sync_async.py: Add comprehensive test suite (9 tests) including empirical timing test that proves non-blocking behavior Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
- temperature → ModelOption.TEMPERATURE - max_tokens → ModelOption.MAX_NEW_TOKENS - seed → ModelOption.SEED Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
With async added, need to fix the catch Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
Fix for -- Error: function raises but has no Raises section Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
* filter out model, n, user, and extra * comment the filtering * use a map and ModelOption.replace_keys() for mapping * fix replace_keys to no-op with from == to Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
* Use whole seconds for timing test so it should be less flakey * Return a real ModelOutputThunk instead of a misleading Mock * Use AsyncMock with side_effect for consistency Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
Contributor
Author
|
Thanks @planetf1 for the review! I fixed each. Please see the comment/question about whether we really want an allowlist. I interpreted that as not the main objective there (but I have a feeling I read it with bias). Happy to address it if you think we should pursue that right now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Misc PR
Type of PR
Description
Testing