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 pandas-stubs to typing dependencies #15849

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented Jan 10, 2024

We have pandas as an optional dependency. This PR ensures type checkers understand Pandas + Astropy.
All the contents of typing are NOT runtime dependencies. The only cost this dependency incurs is when installing with astropy[all], which picks up on all dependencies, not just runtime ones. Since we have Pandas already and this stubs package is maintained by Pandas, this is a very safe type-checking dependency to add.

Found when testing #15794.

If you run mypy then it notices that there are a number of missing stubs dependencies. I think that we should at minimum have type-check level dependencies for our runtime packages (followup PR for types-PyYAML) but also for important optional dependencies. Chances are good that users who care about typing will have these stubs installed already (using e.g. mypy --install-types for automatic discovery) but it's good to be explicit.

Requires #15603

Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@nstarman nstarman added this to the v6.1.0 milestone Jan 10, 2024
@nstarman nstarman marked this pull request as ready for review January 10, 2024 19:29
@pllim pllim requested a review from taldcroft January 10, 2024 21:48
@pllim pllim added the table label Jan 10, 2024
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good to me. I was not familiar with pandas-stubs but I looked into it and it seems like a good thing.

Copy link
Member

@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

If we have a typing set of dependencies we might as well make use of it, but if we want to have more stubs than just pandas then I don't see why we should open separate pull requests for all of them.

@taldcroft
Copy link
Member

@eerovaher - perfect is the enemy of good. The idea is making incremental improvements that we agree move the project forward and not stalling progress in the pursuit of perfection.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Am OK with doing this. But just so I understand the purpose of the typing dependency: is the idea here that when one develops with type checks enabled, we want to have the typing information for all optional dependencies as well, so that one does not get spurious errors? Is there a situation where it would make sense to have separate typing selections for just the minimal and/or recommended packages?

Anyway, overall, this seems fine. On the number of PRs: I assume there are not that many packages that separate out their typing entirely, so it seems fine to me to add those as they are encountered going through various astropy pieces.

@nstarman
Copy link
Member Author

Anyway, overall, this seems fine. On the number of PRs: I assume there are not that many packages that separate out their typing entirely, so it seems fine to me to add those as they are encountered going through various astropy pieces.

Yes. Add as we encounter.

But just so I understand the purpose of the typing dependency: is the idea here that when one develops with type checks enabled, we want to have the typing information for all optional dependencies as well, so that one does not get spurious errors?

Yes. The best solution would be to somehow automate the discovery and installation of stubs without having to manually specify. This might be possible if we commit to a specific type checker and incorporate it into our CI. e.g. mypy has mypy --install-types to do this discovery and installation. There are considerations even then: do we want to support development without having to run tox or pre-commit locally to get mypy. What about integrating with IDEs. IMO adding the stub dependencies is the safest bet now and worth revisiting as we further develop our CI.

Is there a situation where it would make sense to have separate typing selections for just the minimal and/or recommended packages?

IDK. Certainly worth considering. Given the non-runtime nature of the dependencies I think we can safely punt those details for now.

@pllim
Copy link
Member

pllim commented Jan 11, 2024

I think it is. I think this stub is not compatible with oldest numpy we support?

@nstarman
Copy link
Member Author

nstarman commented Jan 11, 2024

Thanks at @pllim for the diagnostic!
Sigh. Cuz of course.
I'll check if there's an older version of the stub we can install.

@nstarman
Copy link
Member Author

nstarman commented Jan 11, 2024

WTF. Is there any version of pandas-stubs that works?

@nstarman
Copy link
Member Author

Ok. Let's wait to add this until py3.10+. I'll move the dependency back to pandas-stubs>=1.4.

@pllim pllim modified the milestones: v6.1.0, v7.0.0 Jan 11, 2024
@pllim pllim marked this pull request as draft January 11, 2024 19:41
@pllim
Copy link
Member

pllim commented Jan 11, 2024

Moving this back to draft then. Thanks for trying!

Signed-off-by: nstarman <nstarman@users.noreply.github.com>
@nstarman nstarman marked this pull request as ready for review April 16, 2024 22:16
@nstarman nstarman requested a review from eerovaher April 16, 2024 22:16
pyproject.toml Outdated
@@ -69,7 +69,8 @@ recommended = [
"matplotlib>=3.3,!=3.4.0,!=3.5.2",
]
typing = [
"typing_extensions>=4.0.0"
"typing_extensions>=4.0.0",
"pandas-stubs>=1.4",
Copy link
Member

Choose a reason for hiding this comment

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

This is adding a version requirement to pandas-stubs, but we don't have a version requirement for pandas itself:

"pandas",

Should we set a minimum supported version for pandas too and am I right in guessing that the pandas-stubs version matches the pandas version it is providing the stubs for?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably actually limit our support of Pandas to >2? https://pypi.org/project/pandas/
@pllim, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

According to the chart in https://scientific-python.org/specs/spec-0000/ , we still should support 1.5.0 until end of 2024?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof. Ok! Then I can push a different PR for min pandas version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants