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

ci: added typechecking #1537

Conversation

ErikBjare
Copy link
Contributor

@ErikBjare ErikBjare commented Jan 20, 2024

While integrating LiteLLM into gptme (ErikBjare/gptme#14), I found that it doesn't properly support typechecking (no py.typed marker file present).

This fixes that, and adds a typechecking workflow to CI.

It also adds a bunch of types-* packages as dev deps. If this is really not wanted (although I don't see why not), it can be replaced by a call to mypy --install-types in CI (but this would require contributors to run the same locally if they want to typecheck).

Copy link

vercel bot commented Jan 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 20, 2024 4:07pm

@ErikBjare
Copy link
Contributor Author

This is now passing and ready to merge!

@krrishdholakia
Copy link
Contributor

Hey @ErikBjare this shows an empty file. Is that expected?

Screenshot 2024-02-17 at 10 55 11 AM

Sorry it took me so long to get to this PR

@ErikBjare
Copy link
Contributor Author

@krrishdholakia Yes, see https://peps.python.org/pep-0561/ for the spec defining its purpose/meaning.

@jamesbraza
Copy link
Contributor

Just reading this PR, @krrishdholakia just to share for your knowledge, the py.typed file is correct here, it's a marker file (presence/absense) so it's contents doesn't matter (normally it's empty)

@@ -55,6 +58,14 @@ litellm = 'litellm:run_server'
flake8 = "^6.1.0"
black = "^23.12.0"
pytest = "^7.4.3"
mypy = "^1.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @ErikBjare @jamesbraza is it necessary to add mypy and types in our core dependencies?

for context i'm looking at openai's dependencies and i don't see this there - https://github.com/openai/openai-python/blob/54a5911f5215148a0bdeb10e2bcfb84f635a75b9/pyproject.toml#L10

Copy link
Contributor

Choose a reason for hiding this comment

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

So these are not the core deps, they're dev (developer) deps. Also, #4136 added mypy to the dev group already, so if this PR were rebased atop main this particular line would go away.

However, the below type stubs added need to stay in dev group dependencies.

Tho I don't think the type stubs should be so specifically pinned, it's okay with something like >= instead of ^

@ErikBjare
Copy link
Contributor Author

Any plans to get something like this merged? I am still not using LiteLLM in my projects due to the poor typing.

@krrishdholakia krrishdholakia changed the base branch from main to litellm_stable_dev_09_26_2024 September 26, 2024 16:12
@krrishdholakia
Copy link
Contributor

Closing this pr, as litellm already has a py.typed file -

# See PEP 561 for more information.

@ErikBjare
Copy link
Contributor Author

@krrishdholakia This PR did much more than simply add the py.typed file.

For the file to make any sense you have to also typecheck your code, which you still do not seem to do?

@krrishdholakia
Copy link
Contributor

Hi @ErikBjare yep we do -

name: Linting Testing

Let me know if i'm missing anything

@ErikBjare
Copy link
Contributor Author

ErikBjare commented Sep 27, 2024 via email

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.

3 participants