Closed ProKil closed 1 week ago
Attention: Patch coverage is 55.55556%
with 20 lines
in your changes missing coverage. Please review.
Project coverage is 60.03%. Comparing base (
bf0321b
) to head (34e8bc3
).
@@ Coverage Diff @@
## main #116 +/- ##
==========================================
+ Coverage 59.67% 60.03% +0.35%
==========================================
Files 46 47 +1
Lines 2455 2402 -53
==========================================
- Hits 1465 1442 -23
+ Misses 990 960 -30
Files | Coverage Δ | |
---|---|---|
sotopia/agents/__init__.py | 100.00% <ø> (ø) |
|
sotopia/envs/evaluators.py | 90.44% <100.00%> (-0.07%) |
:arrow_down: |
sotopia/generation_utils/__init__.py | 100.00% <100.00%> (ø) |
|
sotopia/agents/generate_agent_background.py | 23.33% <66.66%> (ø) |
|
sotopia/server.py | 0.00% <0.00%> (ø) |
|
tests/generation_utils/test_generation.py | 68.42% <50.00%> (-8.51%) |
:arrow_down: |
sotopia/agents/llm_agent.py | 33.98% <28.57%> (+0.34%) |
:arrow_up: |
sotopia/generation_utils/generate.py | 51.80% <50.00%> (+1.15%) |
:arrow_up: |
sotopia/generation_utils/sync.py | 65.00% <65.00%> (ø) |
It is hard to maintain two versions of the same functions side-by-side. Are there specific use of sync functions that you are thinking about?
Since most of sync apis are pure function (i.e. no side effects), we could easily execute them in a process/thread executor in a sync context. Do you need some documentation or examples?
Normally I would separate this into two PRs: one for upgrading langchain, and another for removing sync apis. But I found that there are some weird mypy errors only exist for the sync apis after upgrading langchain. So I think it might make sense to just use this PR to solve these two problems together.
@lwaekfjlk I could re-implement all of the sync apis as wrappers of the corresponding async ones, but do you think it is helpful to provide that in the sotopia package?
Got your point. I just think that considering the difficulty of using sotopia package, probably we need to provide a sync api wrapped out of the async ones. Otherwise, imagine you are a beginner of using sotopia package and what to do some intial test, you would find there is no simple sync API (which you normally use for OpenAI or other things) but only those async ones that a beginner might not be familiar with.
That can be kind of confusing.
Cool! Would you mind adding some use cases?
where would be a good place to add? The notebook demo?
You can add a failed test case and I can implement that to pass the test case
sure, I will do that after work.
sure, I will do that after work.
Thank you so much! :) 🤝
@lwaekfjlk I just added a helper function async_to_sync
which can convert any async function to a sync one with type safety.
cool, looks good
📑 Description
This PR does several things:
✅ Checks
type/descript
(e.g.feature/add-llm-agents
)This change will be reflected in the new version of documentation.
ℹ Additional Information