-
Notifications
You must be signed in to change notification settings - Fork 37
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
Split up /call
and /logs
.
#1152
base: 09-17-Pull_out_env_installation_into_helper_function
Are you sure you want to change the base?
Split up /call
and /logs
.
#1152
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: runhouse/servers/http/http_client.py
Did you find this useful? React with a 👍 or 👎 |
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
runhouse/servers/http/http_server.py
Outdated
@@ -68,6 +71,9 @@ | |||
|
|||
app = FastAPI(docs_url=None, redoc_url=None) | |||
|
|||
# TODO: Better way to store this than a global here? | |||
running_futures: Dict[str, Future] = {} |
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.
not sure if it's really any better, but could store it in the FastAPI app state, since this is only useful for the life of the server anyway right?
@@ -461,6 +462,32 @@ async def get_call( | |||
serialization=serialization, | |||
) | |||
|
|||
# `/logs` POST endpoint that takes in request and LogParams | |||
@staticmethod | |||
@app.get("/logs/{run_name}/{serialization}") |
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.
what serialization options are relevant for logs?
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.
in the event of exception we still need to serialize with the expected type
runhouse/servers/http/http_server.py
Outdated
@@ -68,6 +71,9 @@ | |||
|
|||
app = FastAPI(docs_url=None, redoc_url=None) | |||
|
|||
# TODO: Better way to store this than a global here? | |||
running_futures: Dict[str, Future] = {} |
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.
small nit - isn't this technically storing a Task object
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.
right, good point
result = handle_response( | ||
resp, output_type, error_str, log_formatter=self.log_formatter | ||
with ThreadPoolExecutor() as executor: | ||
# Run logs request in separate thread. Can start it before because it'll wait 5 seconds for the |
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.
maybe i missed this but where are is the delay for the calls request to begin?
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.
There's no delay for that... in the /logs
endpoint server side, there's a delay where it waits for the call
to start.
fe8cb72
to
9bee76b
Compare
7cb3b41
to
6d77b0e
Compare
9bee76b
to
6b66f66
Compare
6d77b0e
to
8a9a1aa
Compare
6b66f66
to
04232f8
Compare
8a9a1aa
to
d0decb7
Compare
04232f8
to
1a4b9ca
Compare
d0decb7
to
3e892d6
Compare
1a4b9ca
to
cde74ae
Compare
3e892d6
to
47fa18b
Compare
f2d9f49
to
80cb59e
Compare
80cb59e
to
ec4ac45
Compare
ec4ac45
to
d43439a
Compare
d43439a
to
6aa7cbc
Compare
No description provided.