-
Notifications
You must be signed in to change notification settings - Fork 75
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
TrussServer supports request/repsonse #1148
Conversation
2c10e41
to
eafad4e
Compare
eafad4e
to
335bbbe
Compare
25e67cf
to
9d3e562
Compare
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.
this is AWESOME. Very excited for these changes -- particularly enabling SSE (which we'll get on day 1), and the stacktrace change (this has been bothering me for a long time, glad we finally got around to it).
took a first pass w a couple questions, will take a closer look later
@@ -974,3 +874,275 @@ def enable_gpu_fn(conf): | |||
# But make sure traces have parents at all. | |||
assert len(user_parents) > 3 | |||
assert len(truss_parents) > 3 | |||
|
|||
|
|||
# Returning Response Objects ########################################################### |
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.
One thought -- do you think it makes sense to have an integration test for the stack trace we made in this PR?
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 was too lazy to lookup the traceback API and the initial implementation was totally bloated by chat GPT - I had this feeling that it could be simplified, now it's pretty straightforward.
But adding a test would be good too!
aa764b8
to
ef153e4
Compare
ef153e4
to
c5e0d36
Compare
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.
lgtm! not sure what these junit test report failures are
7d39fde
to
ba1de60
Compare
🚀 What
Support access to requests object in truss model and allow returning response objects. This follows the internal proposal.
Includes:
ModelDefinitionError
is raised.truss_server.py
inference_server.py
andmodel_wrapper.py
:truss_server
contains the actual server implementation, as an application entrypoint a super shallowmain.py
file is used.ModelDescriptor
that is just initialized once and contains information whether methods are present and sync/async etc.Side quests:
intercept_exceptions
function wrapper with contextmanager so it can be easier used on non-function code sections.importlib.util.module_from_spec
instead ofimportlib.import_module
because it has side effects in unit/integration tests.model_wrapper
loading. - Root cause for prior failure was import side effects (previous point).test_model_wrapper_streaming_timeout
that had missing@pytest.mark.anyio
.This PR includes changes from #1103, which would be obsolete.
💻 How
🔬 Testing