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: mypy for all type check #10921

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented Nov 21, 2024

Summary

This maybe a huge Pull Request, the main goal is to make all the mypy type check passed to fix the potential bugs
So its still WIP and will be draft. and try to close #10928

the first steps had beed done. -> let mypy work in api folder.
And we fixed some of them,

you can check by

  • gh pr checkout 10921

  • when found small bug fix in main branch first

  • install mypy

  • cd api && mypy .

  • make it work for mypy

  • add ci for it

  • always merge main for the latest code

  • fixed them folder by folder, maybe type by type

for now after this commit it still has 4261 errors

some details update here:

image

@laipz8200 @crazywoola

Tip

Close issue syntax: Fixes #<issue number> or Resolves #<issue number>, see documentation for more details.

Screenshots

Before: After:
... ...

Checklist

Important

Please review the checklist below before submitting your pull request.

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

@laipz8200
Copy link
Member

It looks like a great piece of work! Just so you know, due to some historical reasons, the type annotations in the current codebase are not always accurate. Also, extra caution is needed when dealing with dict types from requests or the database.

If you need any assistance, feel free to reach out to me.

@laipz8200
Copy link
Member

Additionally, according to our Contribution Guide, I recommend linking an issue to this PR to clarify your objective and avoid others submitting duplicate PRs.

@yihong0618
Copy link
Contributor Author

It looks like a great piece of work! Just so you know, due to some historical reasons, the type annotations in the current codebase are not always accurate. Also, extra caution is needed when dealing with dict types from requests or the database.

If you need any assistance, feel free to reach out to me.

yes, maybe you can also push commit to this too, for the fixing them all, and I would create an issue for it

@laipz8200
Copy link
Member

For Flask-SQLAlchemy, I’d like to share our plan with you:

  1. Replace the direct use of db.session with Session from sqlalchemy.orm for queries and updates.
  2. In a future major release, we plan to have database models directly inherit from SQLAlchemy’s base class.
  3. Finally, we aim to use SQLAlchemy directly instead of Flask-SQLAlchemy.

As a result, when encountering type-related issues, we can proceed with change 1 and temporarily ignore the errors caused by change 2.

@yihong0618
Copy link
Contributor Author

yihong0618 commented Nov 21, 2024

For Flask-SQLAlchemy, I’d like to share our plan with you:

  1. Replace the direct use of db.session with Session from sqlalchemy.orm for queries and updates.
  2. In a future major release, we plan to have database models directly inherit from SQLAlchemy’s base class.
  3. Finally, we aim to use SQLAlchemy directly instead of Flask-SQLAlchemy.

As a result, when encountering type-related issues, we can proceed with change 1 and temporarily ignore the errors caused by change 2.

yes, this time I also choose ignore it, thanks for the info
and fell free to commit in this pull request~

@bowenliang123
Copy link
Contributor

With my -1 for this PR.

  • mypy for static type check brings little benefits in project
  • the in-line suppressions puzzles the developers, for what rule it's violating or how we could further fix it.
  • no proper auto fixes tool to align with the mypy rules
  • too many suppressions in this PR
  • for the field types, suggest to use Pydantic which runs check in runtime

@yihong0618
Copy link
Contributor Author

With my -1 for this PR.

  • mypy for static type check brings little benefits in project
  • the in-line suppressions puzzles the developers, for what rule it's violating or how we could further fix it.
  • no proper auto fixes tool to align with the mypy rules
  • too many suppressions in this PR
  • for the field types, suggest to use Pydantic which runs check in runtime

some are right, but for now, I checked many of the bugs caused by the type, if not fix it by now, the code base grows, it will never be fixed.

@yihong0618
Copy link
Contributor Author

With my -1 for this PR.

  • mypy for static type check brings little benefits in project
  • the in-line suppressions puzzles the developers, for what rule it's violating or how we could further fix it.
  • no proper auto fixes tool to align with the mypy rules
  • too many suppressions in this PR
  • for the field types, suggest to use Pydantic which runs check in runtime

some are right, but for now, I checked many of the bugs caused by the type, if not fix it by now, the code base grows, it will never be fixed.

but this part I totally agree maybe even all the code happy, it should not in the CI, its too still some pain for everyone work on the mypy, so keep it on develop side maybe better

the in-line suppressions puzzles the developers, for what rule it's violating or how we could further fix it.
no proper auto fixes tool to align with the mypy rules

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.

make type hint passed by mypy check for all the codebase in api/
3 participants