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: FastAPI Implementation of Sotopia Part Two (w websocket) #252

Merged
merged 26 commits into from
Dec 5, 2024

Conversation

XuhuiZhou
Copy link
Member

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

@XuhuiZhou XuhuiZhou changed the title Feature/sotopia UI fastapi websocket feat: FastAPI Implementation of Sotopia Part Two (w websocket) Nov 20, 2024
@XuhuiZhou XuhuiZhou added the ui label Nov 20, 2024
@XuhuiZhou XuhuiZhou marked this pull request as draft November 20, 2024 23:33
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 47.70318% with 148 lines in your changes missing coverage. Please review.

Project coverage is 72.16%. Comparing base (61f190e) to head (67eb180).
Report is 1 commits behind head on demo.

Files with missing lines Patch % Lines
sotopia/ui/fastapi_server.py 35.76% 97 Missing ⚠️
sotopia/ui/websocket_utils.py 49.33% 38 Missing ⚠️
sotopia/server.py 77.19% 13 Missing ⚠️
@@            Coverage Diff             @@
##             demo     #252      +/-   ##
==========================================
- Coverage   74.47%   72.16%   -2.31%     
==========================================
  Files          61       62       +1     
  Lines        3162     3392     +230     
==========================================
+ Hits         2355     2448      +93     
- Misses        807      944     +137     
Files with missing lines Coverage Ξ”
sotopia/server.py 45.29% <77.19%> (+3.62%) ⬆️
sotopia/ui/websocket_utils.py 49.33% <49.33%> (ΓΈ)
sotopia/ui/fastapi_server.py 58.47% <35.76%> (-40.48%) ⬇️

@bugsz
Copy link
Contributor

bugsz commented Nov 21, 2024

A simple usage of this api, still working on it: sotopia-lab/sotopia-demo#14

@bugsz bugsz marked this pull request as ready for review December 1, 2024 20:58
@XuhuiZhou XuhuiZhou requested a review from ProKil December 2, 2024 21:18
pyproject.toml Outdated Show resolved Hide resolved
sotopia/ui/README.md Outdated Show resolved Hide resolved
@ProKil
Copy link
Member

ProKil commented Dec 4, 2024

Please merge main into this branch.

sotopia/ui/README.md Outdated Show resolved Hide resolved
sotopia/ui/README.md Outdated Show resolved Hide resolved
@XuhuiZhou XuhuiZhou requested a review from ProKil December 4, 2024 04:25
@ProKil
Copy link
Member

ProKil commented Dec 5, 2024

Attention: Patch coverage is 40.17857% with 201 lines in your changes missing coverage.

Could you fix this?

@XuhuiZhou
Copy link
Member Author

Attention: Patch coverage is 40.17857% with 201 lines in your changes missing coverage.

Could you fix this?

okay, let's not worry about this for now in light of the timeline

@ProKil ProKil changed the base branch from main to demo December 5, 2024 04:51
@ProKil
Copy link
Member

ProKil commented Dec 5, 2024

Since this PR is very large and not covered by CI tests, it is hard to take it directly into the main branch, i.e. I could not read every line to make sure it doesn't break anything or rely on the test cases to make sure the features introduced will not be broken.

To accelerate the development process, I changed the base to a different branch demo. Please merge related improvements to that branch, and we will merge it whenever it has been thoroughly tested.

@ProKil ProKil merged commit 693f792 into demo Dec 5, 2024
9 checks passed
@ProKil ProKil deleted the feature/sotopia-ui-fastapi-websocket branch December 5, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants