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

Implement mypy plugin for luigi.Task #3315

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Conversation

hiro-o918
Copy link
Contributor

@hiro-o918 hiro-o918 commented Sep 20, 2024

  • Add Mypy Plugin to type-check arguments of luigi.Task

Description

  • Implement a plugin for mypy inspired by dataclass

Note

  • bump up sphinx version and use Python 3.10 for docs environment
    • because build failed on 3.9
    • 9b2e890
  • After writing tests, Python 3.12 failed on mypy errros.

Motivation and Context

This PR aims to solve the following issues.

  • the constructor of luigi.Task cannot be type-checked when initializing it.
  • The following type annotation is invalid since luigi.Parameter() is not str
class Task:
  foo: str = luigi.Parameter()

Have you tested this? If so, how?

  • This PR includes the Mypy tests and docs

tox-env: flake8
- python-version: 3.9
- python-version: "3.10"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use python 3.10 because docs build failed on 3.9
python for flake 8 also updated as a represented environment

]
)

self.assertIn(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part shows errors when assign invalid type for task arguments

@hiro-o918 hiro-o918 marked this pull request as ready for review September 20, 2024 18:20
@hiro-o918 hiro-o918 requested review from dlstadther and a team as code owners September 20, 2024 18:20
@hiro-o918 hiro-o918 marked this pull request as draft September 21, 2024 16:07
dlstadther
dlstadther previously approved these changes Sep 22, 2024
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

LGTM. Just a mention/question about python versions

#
# However, we also want attributes defined in the subtype to override ones defined
# in the parent. We can implement this via a dict without disrupting the attr order
# because dicts preserve insertion order in Python 3.7+.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Luigi still advertises to be 3.6 compatible. Though, support for 3.6 should be removed (readme reference, tox reference, github actions test execution, etc.).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw elsewhere in this PR that the mypy stuff requires 3.8 or later. What is the behavior when someone tries to use it with 3.6 or 3.7?

Copy link
Contributor Author

@hiro-o918 hiro-o918 Sep 23, 2024

Choose a reason for hiding this comment

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

I saw elsewhere in this PR that the mypy stuff requires 3.8 or later

Sorry about your confusing.
I made a mistake to set a type annotations like list[Any] which supported after 3.8
I fixed this issue, the tests worked on 3.6 and 3.7.
I did not check python 3.6 and 3.7 on the tests.

I'll check how it works on Python 3.6 via tests.
If the test become flaky, one of workaround is ommit supporting mypy plugion on 3.6

Copy link
Contributor Author

@hiro-o918 hiro-o918 Sep 23, 2024

Choose a reason for hiding this comment

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

After inspecting issues about python 3.6 and 3.7, some built-in features are lacking (e.g. some type annotation features like Final and Literal as well as dict behaviors).

Can I want to omit these versions for this plugin because this feature is not core required to use luigi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, i'm fine with the included solution (requiring a minimum python version to import and use the mypy plugin).

@dlstadther
Copy link
Collaborator

For some reason, the 3.9 version of docs and flake8 are being expected. Although I've approved the PR and am a maintainer of the project, I'm not being allowed to click merge due to 2 statuses missing.

@dlstadther
Copy link
Collaborator

To merge, we may require a Spotify employee who has the appropriate permissions. I think the CI is trying to wait on some of the tests expected by the master branch even though those same tests were updated to 3.10 and are running successfully.

@RRap0so , would you be able to look and possibly merge anyway? (I cannot merge, the button is grayed-out saying Required statuses must pass before merging).

@RRap0so RRap0so merged commit 829fc0c into spotify:master Sep 24, 2024
48 checks passed
@RRap0so
Copy link
Contributor

RRap0so commented Sep 24, 2024

@dlstadther all done 👍

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