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

Type stubs for tqdm library #8235

Merged
merged 40 commits into from Jul 5, 2022
Merged

Type stubs for tqdm library #8235

merged 40 commits into from Jul 5, 2022

Conversation

Gilthans
Copy link
Contributor

@Gilthans Gilthans commented Jul 4, 2022

While tqdm is very popular, it is not only not typed, but strips the types of sequences it receives due to the way it works.
This should resolve the issue for most users.

An issue to add type hints to tqdm has been around since 2016, but hasn't seen any progress.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 4, 2022

It's fine to use Literal in typeshed, you just have to import it from typing_extensions instead of typing. (In typeshed we pretend typing_extensions is part of the stdlib for Reasons.)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! We'll take a full look when the tests are passing (feel free to ask if you need a hand with any of them).

Please don't make unrelated changes to CONTRIBUTING.md in this PR, though. If you think that needs to be changed, please do that in its own PR. It'll make for a very confusing git history otherwise.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

If the implementation breaks LSP by having an incompatible override, the best thing to do is to mimic the implementation exactly and add a # type: ignore[override] to silence mypy on the line in question

stubs/tqdm/tqdm/tk.pyi Outdated Show resolved Hide resolved
stubs/tqdm/tqdm/utils.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@Gilthans Gilthans requested a review from AlexWaygood July 5, 2022 15:36
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/tqdm_logger.py:141: error: Incompatible types in assignment (expression has type "tqdm[<nothing>]", variable has type "None")  [assignment]

optuna (https://github.com/optuna/optuna)
+ optuna/progress_bar.py:57: error: Need type annotation for "_progress_bar"
+ optuna/progress_bar.py:67: error: Argument 1 to "format_interval" of "tqdm" has incompatible type "Optional[float]"; expected "float"
+ optuna/integration/_lightgbm_tuner/optimize.py:622: error: Need type annotation for "pbar"

rclip (https://github.com/yurijmikhalevich/rclip)
- rclip/main.py:8: error: Cannot find implementation or library stub for module named "tqdm"

pyppeteer (https://github.com/pyppeteer/pyppeteer)
+ pyppeteer/chromium_downloader.py:92: error: Need type annotation for "process_bar"
+ pyppeteer/chromium_downloader.py:94: error: Argument "file" to "tqdm" has incompatible type "Optional[str]"; expected "Optional[SupportsWrite[str]]"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/tqdm_logger.py:141: error: Incompatible types in assignment (expression has type "tqdm[<nothing>]", variable has type "None")  [assignment]

optuna (https://github.com/optuna/optuna)
+ optuna/progress_bar.py:57: error: Need type annotation for "_progress_bar"
+ optuna/progress_bar.py:67: error: Argument 1 to "format_interval" of "tqdm" has incompatible type "Optional[float]"; expected "float"
+ optuna/integration/_lightgbm_tuner/optimize.py:622: error: Need type annotation for "pbar"

rclip (https://github.com/yurijmikhalevich/rclip)
- rclip/main.py:8: error: Cannot find implementation or library stub for module named "tqdm"

pyppeteer (https://github.com/pyppeteer/pyppeteer)
+ pyppeteer/chromium_downloader.py:92: error: Need type annotation for "process_bar"
+ pyppeteer/chromium_downloader.py:94: error: Argument "file" to "tqdm" has incompatible type "Optional[str]"; expected "Optional[SupportsWrite[str]]"

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/tqdm_logger.py:141: error: Incompatible types in assignment (expression has type "tqdm[Any]", variable has type "None")  [assignment]

optuna (https://github.com/optuna/optuna)
+ optuna/progress_bar.py:57: error: Need type annotation for "_progress_bar"
+ optuna/progress_bar.py:67: error: Argument 1 to "format_interval" of "tqdm" has incompatible type "Optional[float]"; expected "float"

rclip (https://github.com/yurijmikhalevich/rclip)
- rclip/main.py:8: error: Cannot find implementation or library stub for module named "tqdm"

pyppeteer (https://github.com/pyppeteer/pyppeteer)
+ pyppeteer/chromium_downloader.py:94: error: Argument "file" to "tqdm" has incompatible type "Optional[str]"; expected "Optional[SupportsWrite[str]]"

@AlexWaygood
Copy link
Member

(I might need to iterate a little bit to tackle these primer errors)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

ignite (https://github.com/pytorch/ignite)
+ ignite/contrib/handlers/tqdm_logger.py:141: error: Incompatible types in assignment (expression has type "tqdm[Any]", variable has type "None")  [assignment]

optuna (https://github.com/optuna/optuna)
+ optuna/progress_bar.py:67: error: Argument 1 to "format_interval" of "tqdm" has incompatible type "Optional[float]"; expected "float"

rclip (https://github.com/yurijmikhalevich/rclip)
- rclip/main.py:8: error: Cannot find implementation or library stub for module named "tqdm"

pyppeteer (https://github.com/pyppeteer/pyppeteer)
+ pyppeteer/chromium_downloader.py:94: error: Argument "file" to "tqdm" has incompatible type "Optional[str]"; expected "Optional[SupportsWrite[str]]"

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for your patience! The remaining mypy_primer hits all look like true positives to me (except the pyppeteer one, which I can't make head or tail of... but you can't catch them all ¯\_(ツ)_/¯)

@AlexWaygood AlexWaygood merged commit c23cb7b into python:master Jul 5, 2022
@JelleZijlstra
Copy link
Member

Weird, https://github.com/pyppeteer/pyppeteer/blob/f98f9030d3d40dc629bf08f93672f05caf8dfb0e/pyppeteer/chromium_downloader.py#L94 doesn't have a tqdm line. The next line does, but there's no file argument. Is mypy-primer looking at a different version of the library? The file hasn't changed since January.

@AlexWaygood
Copy link
Member

Weird, https://github.com/pyppeteer/pyppeteer/blob/f98f9030d3d40dc629bf08f93672f05caf8dfb0e/pyppeteer/chromium_downloader.py#L94 doesn't have a tqdm line. The next line does, but there's no file argument. Is mypy-primer looking at a different version of the library? The file hasn't changed since January.

Yeah, I have no idea. Weirdly, the default branch for pyppeteer seems to be dev rather than master — but the error doesn't make sense on master either.

@JelleZijlstra
Copy link
Member

There's a hardcoded revision: https://github.com/hauntsaninja/mypy_primer/blob/d91de1f533963a41d362531147c2ff407c286433/mypy_primer.py#L1431

This is the version it's checking: https://github.com/pyppeteer/pyppeteer/blob/2d27bfdf9b6d0df32e5ebe869b057409042417a8/pyppeteer/chromium_downloader.py#L94
It's passing os.devnull which is indeed a str, not a SupportsWrite[str]. I don't know whether that code is just broken.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 5, 2022

There's a hardcoded revision: https://github.com/hauntsaninja/mypy_primer/blob/d91de1f533963a41d362531147c2ff407c286433/mypy_primer.py#L1431

This is the version it's checking: https://github.com/pyppeteer/pyppeteer/blob/2d27bfdf9b6d0df32e5ebe869b057409042417a8/pyppeteer/chromium_downloader.py#L94 It's passing os.devnull which is indeed a str, not a SupportsWrite[str]. I don't know whether that code is just broken.

Hmm, seems like primer's been hardcoded to that revision for 2 years now. I wonder if tqdm has changed in the meantime so that str is no longer good to be passed to the file parameter. It doesn't look to me like file is meant to be a str (but I haven't tried it, just read the source code on the master branch)

JelleZijlstra added a commit to hauntsaninja/mypy_primer that referenced this pull request Jul 5, 2022
python/typeshed#8235 (comment)

It's not clear from the comment why this was hardcoded. Let's use this PR to figure out whether we can drop the pin.
@Gilthans
Copy link
Contributor Author

Gilthans commented Jul 6, 2022

Awesome to see this go in! Thanks a lot @AlexWaygood for making contributing so smooth and welcoming!

hauntsaninja pushed a commit to hauntsaninja/mypy_primer that referenced this pull request Jul 6, 2022
python/typeshed#8235 (comment)

It's not clear from the comment why this was hardcoded.
@alkatar21
Copy link

alkatar21 commented Jul 7, 2022

@AlexWaygood Thanks for your work, that really cool to finally have typings for it.
I do not know if I should create a new issue or if it will be seen here. Anyway, under tqdm.contrib.logging are 2 context managers that are currently annotated as iterator and therefore generate the following messages:

    with logging_redirect_tqdm():
        pass
"Iterator[None]" has no attribute "__enter__"; maybe "__iter__"?  [attr-defined] mypy(error)
"Iterator[None]" has no attribute "__exit__"; maybe "__next__"?  [attr-defined] mypy(error)
Object of type "Iterator[None]" cannot be used with "with" because it does not implement __enter__ Pylance[reportGeneralTypeIssues]
Object of type "Iterator[None]" cannot be used with "with" because it does not implement __exit__ Pylance[reportGeneralTypeIssues]

@AlexWaygood
Copy link
Member

@alkatar21 thanks! I've filed #8251.

@AlexWaygood
Copy link
Member

@alkatar21 thanks! I've filed #8251.

And, merged (thanks for the quick review @srittau!). A new release should be shortly auto-uploaded to PyPI by the typeshed bots :)

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.

None yet

4 participants