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.

Sign up for GitHub

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

Jump to bottom

Lightning: make type hints public #17100

Merged
merged 21 commits into from
Apr 27, 2023
Merged

Lightning: make type hints public #17100

merged 21 commits into from
Apr 27, 2023

Conversation

adamjstewart
Copy link
Contributor

@adamjstewart adamjstewart commented Mar 16, 2023 •
edited
Loading

What does this PR do?

In TorchGeo, we recently switched from import pytorch_lightning to import lightning. I was surprised to see that all types hints were missing. It looks like a py.typed file was added for pytorch_lightning but not for other namespaces. This PR fixes that by adding it to all namespaces.

Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Reviewer checklist
  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

@github-actions github-actions bot added app (removed) Generic label for Lightning App package fabric lightning.fabric.Fabric labels Mar 16, 2023
@adamjstewart
Copy link
Contributor Author

Last time we added this was in #3187. Pinging the same people involved in that PR: @MarioIshac @Borda @rohitgr7

@adamjstewart adamjstewart mentioned this pull request Mar 16, 2023
Merged
@codecov
Copy link

codecov bot commented Mar 16, 2023 •
edited
Loading

Codecov Report

Merging #17100 (353f834) into master (f4b1fc0) will decrease coverage by 0%.
The diff coverage is n/a.

Additional details and impacted files 
@@           Coverage Diff            @@
##           master   #17100    +/-   ##
========================================
- Coverage      83%      82%    -0%     
========================================
  Files         415      415            
  Lines       31657    31657            
========================================
- Hits        26223    26072   -151     
- Misses       5434     5585   +151

@adamjstewart adamjstewart mentioned this pull request Mar 16, 2023
Closed
@awaelchli awaelchli added the community This PR is from the community label Mar 17, 2023
@awaelchli awaelchli added this to the 2.0.x milestone Mar 17, 2023
carmocca
carmocca approved these changes Mar 17, 2023
View reviewed changes
src/lightning_fabric/MANIFEST.in Outdated Show resolved Hide resolved
src/lightning_app/MANIFEST.in Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Mar 17, 2023
@carmocca carmocca enabled auto-merge (squash) March 17, 2023 14:09
@carmocca carmocca assigned carmocca and unassigned Borda Mar 17, 2023
@adamjstewart
Copy link
Contributor Author

Tests seem to be failing so this wasn't auto-merged. Anything I can do to fix these tests? I couldn't tell what was wrong or whether it was just a transient failure.

@adamjstewart
Copy link
Contributor Author

Still failing...

@Borda
Copy link
Member

Borda commented Mar 29, 2023

The wheel that gets created doesn't have them in these positions, it has them in:

yes, well, I can move them back but what is your question? :)

@adamjstewart
Copy link
Contributor Author

My question is that this PR was supposed to add type hints to lightning, but does not add type hints to lightning. Does anyone know how to add type hints to lightning? The build system is quite confusing and I have no idea how things are supposed to work.

In order for mypy to find the type hints, they must be in:

lightning/py.typed

(same for other packages, but I care less about those)

@github-actions github-actions bot removed the pl Generic label for PyTorch Lightning package label Mar 29, 2023
@Borda
Copy link
Member

Borda commented Mar 29, 2023

I see, added it to the manifest

@Borda Borda enabled auto-merge (squash) March 29, 2023 16:57
@adamjstewart
Copy link
Contributor Author

Thanks, I now see lightning/py.typed in the right place! There are also additional py.typed files in subdirectories, but those should be harmlessly ignored.

@adamjstewart
Copy link
Contributor Author

Anyone know how to solve the failing tests? Merging master doesn't seem to be working.

awaelchli
awaelchli reviewed Apr 11, 2023
View reviewed changes
.gitignore Show resolved Hide resolved
@awaelchli
Copy link
Contributor

Let's see if Lightning-Universe/lightning-quick-start#30 helped, otherwise we can consider force-merging it

@Borda
Copy link
Member

Borda commented Apr 11, 2023

Let's see if Lightning-Universe/lightning-quick-start#30 helped, otherwise we can consider force-merging it

That one is falling for a while without any apparent change on the framework side...

@adamjstewart
Copy link
Contributor Author

Just realized that tests on the master branch haven't passed in months. Makes me feel better about this PR I guess. Still alarming...

Any idea if the failing tests here are new and introduced by my PR or old and we can safely merge?

@lexierule lexierule disabled auto-merge April 27, 2023 12:24
@lexierule lexierule merged commit 3d7360a into Lightning-AI:master Apr 27, 2023
@adamjstewart adamjstewart deleted the type-hints branch April 27, 2023 13:19
Borda pushed a commit that referenced this pull request May 9, 2023
@adamjstewart @Borda
Lightning: make type hints public (#17100)
abe969c 
* Add missing MANIFESTs

* move

* one more

* Ignore version.info properly

* move

* manifest

---------

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
(cherry picked from commit 3d7360a)
Borda pushed a commit that referenced this pull request May 9, 2023
@adamjstewart @Borda
Lightning: make type hints public (#17100)
b314ee8 
* Add missing MANIFESTs

* move

* one more

* Ignore version.info properly

* move

* manifest

---------

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
(cherry picked from commit 3d7360a)
Borda pushed a commit that referenced this pull request Jun 1, 2023
@adamjstewart @Borda
Lightning: make type hints public (#17100)
ff88ebc 
* Add missing MANIFESTs

* move

* one more

* Ignore version.info properly

* move

* manifest

---------

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
(cherry picked from commit 3d7360a)
lantiga pushed a commit that referenced this pull request Jun 2, 2023
@adamjstewart @lantiga
Lightning: make type hints public (#17100)
fc016dc 
* Add missing MANIFESTs

* move

* one more

* Ignore version.info properly

* move

* manifest

---------

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
(cherry picked from commit 3d7360a)
@adamjstewart
Copy link
Contributor Author

This is working perfectly in 2.0.3, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmocca carmocca carmocca approved these changes

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@lantiga lantiga Awaiting requested review from lantiga lantiga is a code owner

@hhsecond hhsecond Awaiting requested review from hhsecond

@ethanwharris ethanwharris Awaiting requested review from ethanwharris ethanwharris is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
app (removed) Generic label for Lightning App package community This PR is from the community fabric lightning.fabric.Fabric ready PRs ready to be merged
Projects
None yet
Milestone
2.0.x
Development

Successfully merging this pull request may close these issues.
5 participants
@adamjstewart @awaelchli @Borda @carmocca @lexierule