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

Feat/addtional fast apis for non-streaming simulation and managing relationshio #265

Merged
merged 15 commits into from
Dec 11, 2024

Conversation

XuhuiZhou
Copy link
Member

Add additional fastAPI to align with the Sotopia-S4 paper

Closes #

πŸ“‘ Description

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed
  • Branch name follows type/descript (e.g. feature/add-llm-agents)
  • Ready for code review

β„Ή Additional Information

sotopia/ui/fastapi_server.py Outdated Show resolved Hide resolved
sotopia/ui/fastapi_server.py Outdated Show resolved Hide resolved
sotopia/ui/fastapi_server.py Outdated Show resolved Hide resolved
),
}
)
episode_pk = await arun_one_episode(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this API would only return after the whole episode is run. A better design is to return an episode id immediately, and the user can check the episode's status via following calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add the arun_one_episode into background tasks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Not quite sure if I follow here, are you referring to constantly saving episodes when each msg is produced?

You add the arun_one_episode into background tasks.
what does this mean?

Copy link
Member

@ProKil ProKil Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So right now, the episode_pk is returned after the simulation is done. But this takes long to return to the client. So a better way is to return an episode_pk immediately, and run arun_one_episode in the background. Here is the pseudocode:

@app.post("/simulate/", response_model=str)
async def simulate(simulate_request: SimulateRequest) -> str:
    ...
    episode_pk = EpisodeLog().pk
    asyncio.create_task(arun_one_episode(..., episode_pk=episode_pk)) # update arun_one_episode to take an extra arg: episode_pk
    return episode_pk

async def arun_one_episode(..., episode_pk: str):
    # run simulation
    ....
    # finish simulation
    episode_log.pk = episode_pk
    episode_log.save()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A step after this is to add a simulation status:

class SimulationStatus(JsonModel):
    episode_pk: str = Field(index)
    status: Literal["Started", "Error", "Completed"]

@app.get("/episodes/{episode_pk}")
class get_episode(episode_pk):
    status = SimulationStatus.find(SimulationStatus.episode_pk == episode_pk)
    if status.status in ["Started", "Error"]:
        return ....
    elif status.staus in ["Completed"]:
        return ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the simulation status and made sure every simulation call would return an episode pk

I don't quite follow tho, about the asyncio.create_task one as users won't be able to check the episode until the task is done either way, and the current async implementation of the async def simulate(simulate_request: SimulateRequest) won't block any process?

Like people can still batch the simulate API call?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, when dealing with long-running requests, the standard way is to give client a fast response (202). Here are some references:

  1. https://learn.microsoft.com/en-us/azure/architecture/patterns/async-request-reply
  2. 202 (Accepted) as return code https://datatracker.ietf.org/doc/html/rfc7231#section-6.3.3

Here is what Claude generates for me as an example workflow

POST /api/tasks
> 202 Accepted
{
    "task_id": "abc123",
    "status_url": "/api/tasks/abc123"
}

GET /api/tasks/abc123
> 200 OK
{
    "status": "processing",
    "progress": 45
}

GET /api/tasks/abc123
> 200 OK
{
    "status": "completed",
    "result": { ... }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ProKil I see,

@ProKil
Copy link
Member

ProKil commented Dec 9, 2024

Can you also make the PR name more informative? Like APIs related to what functions?

@XuhuiZhou XuhuiZhou changed the title Feat/addtional fast apis Feat/addtional fast apis for non-streaming simulation and managing relationshio Dec 9, 2024
@XuhuiZhou XuhuiZhou requested a review from ProKil December 9, 2024 19:09
@ProKil ProKil merged commit dea25d3 into demo Dec 11, 2024
1 check passed
@ProKil ProKil deleted the feat/addtional_fast_apis branch December 11, 2024 19:19
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