sotopia-lab / sotopia

Sotopia: an Open-ended Social Learning Environment (ICLR 2024 spotlight)
https://docs.sotopia.world
MIT License
164 stars 20 forks source link

avoid stubs maintance #65

Closed lwaekfjlk closed 6 months ago

lwaekfjlk commented 6 months ago

Closes #64

📑 Description

✅ Checks

ℹ Additional Information

lwaekfjlk commented 6 months ago

follow the settings in vllm.

Link mypy with config-file: https://github.com/vllm-project/vllm/blob/main/.github/workflows/mypy.yaml Ignore import checking: https://github.com/vllm-project/vllm/blob/main/pyproject.toml

lwaekfjlk commented 6 months ago

@ProKil I change and fix a few potential bugs that appears after changing to non-strict mode without stubs.

ProKil commented 6 months ago

I think stub files have their role in mypy check.

lwaekfjlk commented 6 months ago

But the stubs is only related to third-party packages, right? If type errors happen when using stubs, it should raise error from the third-party side instead of from mypy side and it looks like is the common case to avoid adding stubs for mypy?

vllm avoid this: https://github.com/vllm-project/vllm/blob/main/pyproject.toml

[tool.mypy] python_version = "3.8"

ignore_missing_imports = true check_untyped_defs = true

Once we ignore_missing_imports, then stubs file is no need.

lwaekfjlk commented 6 months ago

after switching from strict mode and stubs files to loose model of mypy, I actually came across a few additional type error that I found that are indeed potential bugs. Therefore, it looks like strict mode testing of mypy does not strictly indicate more strict checking.

ProKil commented 6 months ago

I have to ponder a bit more on this. I think one of way is to remove most of the dependencies we currently have, including LangChain.

lwaekfjlk commented 6 months ago

sure. I just fee like large-scale popular github repo does not have stubs in them.

ProKil commented 6 months ago

Based on our discussion offline. We can work on cut the dependencies instead of removing stubs all together.