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 seaborn stubs #10721

Merged
merged 23 commits into from
Oct 28, 2023
Merged

Add seaborn stubs #10721

merged 23 commits into from
Oct 28, 2023

Conversation

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

For the pytype_test.py failures, we'll probably need to make some adjustments to this function:

def _get_pkgs_associated_with_requirement(req_name: str) -> list[str]:

If need be, we could probably just add some hacky special-casing for pandas-stubs to the function, e.g.

if req_name == "pandas-stubs":
    return ["pandas"]

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2023

The mypy_test.py failures are harder to fix :(

We may need to consider adding a requires-python field to our METADATA.toml files, so we could declare that a stubs package only works with e.g. Python 3.9+. If we did that, it would enable us to skip running mypy with Python 3.8 for Seaborn in CI. But it would require also making some changes to the stub_uploader so that it includes the requires-python data in the setup.py files it generates.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@hamdanal
Copy link
Contributor Author

The mypy_test.py failures are harder to fix :(

We may need to consider adding a requires-python field to our METADATA.toml files, so we could declare that a stubs package only works with e.g. Python 3.9+. If we did that, it would enable us to skip running mypy with Python 3.8 for Seaborn in CI. But it would require also making some changes to the stub_uploader so that it includes the requires-python data in the setup.py files it generates.

Thanks for looking into this. Should we open an issue about this to see what other maintainers say?

@AlexWaygood
Copy link
Member

The mypy_test.py failures are harder to fix :(
We may need to consider adding a requires-python field to our METADATA.toml files, so we could declare that a stubs package only works with e.g. Python 3.9+. If we did that, it would enable us to skip running mypy with Python 3.8 for Seaborn in CI. But it would require also making some changes to the stub_uploader so that it includes the requires-python data in the setup.py files it generates.

Thanks for looking into this. Should we open an issue about this to see what other maintainers say?

Yes, that would be great!

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2023

I've triggered a rerun of the stub-uploader tests now typeshed-internal/stub_uploader#102 has been merged.

EDIT: They pass now; just mypy_test.py to go! 🚀

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

All green now 🥳 though I think we should also wait for typeshed-internal/stub_uploader#103 before we can consider merging this

@hamdanal
Copy link
Contributor Author

I'll mark the PR as "Ready for review" because it is ready for review :P.
Maybe we can add a https://github.com/python/typeshed/labels/DO-NOT-MERGE or https://github.com/python/typeshed/labels/deferred label?

@hamdanal hamdanal marked this pull request as ready for review September 24, 2023 19:11
@AlexWaygood AlexWaygood added status: deferred Issue or PR deferred until some precondition is fixed status: DO NOT MERGE and removed status: deferred Issue or PR deferred until some precondition is fixed status: DO NOT MERGE labels Sep 24, 2023
@AlexWaygood
Copy link
Member

typeshed-internal/stub_uploader#103 was merged 🎉

@JelleZijlstra
Copy link
Member

It seems like seaborn has a lot of inline types, and these stubs closely track them. Have you considered asking whether they would ship a py.typed? I don't see any discussion of that in their issue tracker.

@hamdanal hamdanal marked this pull request as draft October 8, 2023 08:28
@hamdanal hamdanal marked this pull request as ready for review October 8, 2023 09:33
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra 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 clarifying, indeed the files outside _core are mostly not annotated. I reviewed up to cm.pyi now, mostly by spot-checking and reading along with the implementation.

stubs/seaborn/seaborn/_stats/regression.pyi Show resolved Hide resolved
stubs/seaborn/seaborn/algorithms.pyi Show resolved Hide resolved
stubs/seaborn/seaborn/axisgrid.pyi Outdated Show resolved Hide resolved
stubs/seaborn/seaborn/axisgrid.pyi Show resolved Hide resolved
stubs/seaborn/seaborn/axisgrid.pyi Show resolved Hide resolved
*,
hue: str | None = None,
vars: Iterable[str] | None = None,
x_vars: Iterable[str] | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

This check seems to indicate a broader type:

        if np.isscalar(x_vars):
            x_vars = [x_vars]

(Same for y_vars)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are documented as "lists of variable names" but I annotated them as iterables because they are casted to list at runtime. The isscalar check seems to allow str to be accepted but this is already covered by Iterable[str]. I think no change is needed here?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't feel great to rely on the fact that str is an Iterable[str], as usually that isn't what you want. As I recall pytype generally disallows str for Iterable[str]. So let's be explicit:

Suggested change
x_vars: Iterable[str] | None = None,
x_vars: Iterable[str] | str | None = None,

stubs/seaborn/seaborn/axisgrid.pyi Outdated Show resolved Hide resolved
stubs/seaborn/seaborn/axisgrid.pyi Outdated Show resolved Hide resolved
stubs/seaborn/seaborn/categorical.pyi Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Got to the end this time! A few more comments.

stubs/seaborn/seaborn/external/appdirs.pyi Show resolved Hide resolved
stubs/seaborn/seaborn/external/husl.pyi Outdated Show resolved Hide resolved
stubs/seaborn/seaborn/palettes.pyi Outdated Show resolved Hide resolved
stubs/seaborn/seaborn/palettes.pyi Show resolved Hide resolved
stubs/seaborn/seaborn/palettes.pyi Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit c189ca0 into python:main Oct 28, 2023
48 checks passed
@JelleZijlstra
Copy link
Member

Thanks for your work @hamdanal!

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