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

[MOD-3841]: run web server in separate thread #2255

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kramstrom
Copy link
Contributor

@kramstrom kramstrom commented Sep 20, 2024

Allows blocking calls in web_server decorated functions so you can do something like:

import modal

image = modal.Image.debian_slim().pip_install("lavague-core", "lavague-server").env({"LAVAGUE_TELEMETRY": "NONE"})
app = modal.App("lavague-server", image=image)


@app.function()
@modal.web_server(port=8000)
def websocket_app():
    from lavague.core import ActionEngine, WorldModel
    from lavague.core.agents import WebAgent
    from lavague.server import AgentSession
    from lavague.server.base import AgentServer
    from lavague.server.driver import DriverServer

    def create_agent(session: AgentSession):
        world_model = WorldModel()
        driver = DriverServer(session)
        action_engine = ActionEngine(driver)
        return WebAgent(world_model, action_engine)

    server = AgentServer(create_agent, port=8000)
    server.serve()

instead of having to run it in a thread, since it can be confusing that the web_server decorator currently expects users to run everything in a background thread

@ekzhang
Copy link
Member

ekzhang commented Sep 20, 2024

If you wanted to use uvicorn.run(), couldn't you just use asgi_app in that case?

Right now web_server is designed to wait until the function finishes executing before it starts polling for the server to start up, this is more flexible than always running the code in a thread and also will propagate initialization errors sooner.

This is also a breaking change since the startup_timeout would start at function begin, not at function end.

@kramstrom
Copy link
Contributor Author

kramstrom commented Sep 20, 2024

If you wanted to use uvicorn.run(), couldn't you just use asgi_app in that case?

Right now web_server is designed to wait until the function finishes executing before it starts polling for the server to start up, this is more flexible than always running the code in a thread and also will propagate initialization errors sooner.

This is also a breaking change since the startup_timeout would start at function begin, not at function end.

Yeah the uvicorn was just an example of something that will block the thread. I think the reasoning for running the function in a thread is that it can be a footgun for users that their web_server decorated function can be blocking and never therefore never return without indicating clearly why

@ekzhang
Copy link
Member

ekzhang commented Sep 20, 2024

Yeah the uvicorn was just an example of something that will block the thread.

Right, I'm just saying that if the concern is ergonomics in eliminating one line of code (Thread().start()), we should design based on examples that exist rather than theoretical examples.

@kramstrom
Copy link
Contributor Author

kramstrom commented Sep 23, 2024

Yeah the uvicorn was just an example of something that will block the thread.

Right, I'm just saying that if the concern is ergonomics in eliminating one line of code (Thread().start()), we should design based on examples that exist rather than theoretical examples.

Yup, that's totally fair, updated the example now to a real user issue that prompted this

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.

2 participants