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

Add mypy to core #14883

Merged
merged 24 commits into from
Sep 2, 2024
Merged

Add mypy to core #14883

merged 24 commits into from
Sep 2, 2024

Conversation

logan-markewich
Copy link
Collaborator

@logan-markewich logan-markewich commented Jul 22, 2024

For a while, mypy has not been running properly.

Ideally we could run it with pants, but that is currently blocked.

So for now, we can at least run on core and keep that up-to-date.

This PR ends up fixing 800+ mypy issues.

Here are some notes:

  • The datasets code had some weird typing issues, had to use ignore more than I wanted
  • Anything needed a pydantic model as a function input/output is borked (had to use ignore)
  • Sequence vs. List was very confusing to annotate. Fixed a few issues, but also used ignore for some
  • SimpleDirectoryReader.aload_file was completely broken it seems like?
  • The draw most recent workflow function also seemed (recently!) broken. I'll need to update the utils package

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 22, 2024
@nerdai
Copy link
Contributor

nerdai commented Jul 23, 2024

For a while, mypy has not been running properly.

Ideally we could run it with pants, but that is currently blocked.

So for now, we can at least run on core and keep that up-to-date. There are many mypy issues (currently down to 699 at the time of writing)

ya pants would be nice. With a past monorepo, I resorted to using multiple mypy invocations with different entry points. But that was for like 2-3 different packages lol. We have over 400 here...

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 29, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Sep 1, 2024
@logan-markewich logan-markewich changed the title [WIP] Add mypy to core Add mypy to core Sep 1, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@logan-markewich logan-markewich merged commit bd025d4 into main Sep 2, 2024
10 checks passed
@logan-markewich logan-markewich deleted the logan/mypy_on_core branch September 2, 2024 15:40
raspawar pushed a commit to raspawar/llama_index that referenced this pull request Sep 4, 2024
raspawar pushed a commit to raspawar/llama_index that referenced this pull request Sep 4, 2024
@@ -701,6 +701,8 @@ def stream_chat(
and chat_response.is_dummy_stream
)
e.on_end(payload={EventPayload.RESPONSE: chat_response})

assert isinstance(chat_response, StreamingAgentChatResponse)
Copy link

Choose a reason for hiding this comment

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

Why is there a need to check the type of chat_response again, when the earlier logic already allows dummy stream chat responses, making this seem redundant or conflicting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mypy was complaining that it wasn't the correct type

I see the issue though since it could technically return AgentChatResponse

Will need a pr to clean up that logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants