Skip to content

Conversation

@HalestormAI
Copy link
Contributor

@HalestormAI HalestormAI commented Jul 21, 2022

Fixes mypy errors attributed to pytorch_lightning/strategies/ipu.py for issue #13445

removed module name from pyproject.toml [tool.mypy.overrides]
resolved discrepencies for `Optional` returned objects
fixed missing and incorrect type hints.

Part of #13445

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

  • Was this discussed/approved 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

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

  • 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

Did you have fun?

Make sure you had fun coding 🙃

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 21, 2022
@HalestormAI
Copy link
Contributor Author

Hmm, ok the mypy test was passing locally, perhaps missed something when I rebased. Will fix.

@HalestormAI HalestormAI marked this pull request as draft July 21, 2022 12:17
@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #13786 (ed96cef) into master (ee4233b) will increase coverage by 15%.
The diff coverage is 62%.

❗ Current head ed96cef differs from pull request most recent head bc6f314. Consider uploading reports for the commit bc6f314 to get more accurate results

@@            Coverage Diff            @@
##           master   #13786     +/-   ##
=========================================
+ Coverage      61%      76%    +15%     
=========================================
  Files         341      341             
  Lines       26564    26620     +56     
=========================================
+ Hits        16214    20239   +4025     
+ Misses      10350     6381   -3969     

@HalestormAI HalestormAI force-pushed the tests/13445_ipu_strategy_typing_annotations branch from 0c9d68a to 44bb884 Compare July 21, 2022 13:10
@otaj otaj mentioned this pull request Jul 21, 2022
52 tasks
@akihironitta akihironitta added code quality community This PR is from the community labels Jul 23, 2022
@otaj
Copy link
Contributor

otaj commented Jul 29, 2022

Hi @HalestormAI, do you need some help with finalizing this PR? 💪

@HalestormAI
Copy link
Contributor Author

Yes please - went round in circles a few times trying to resolve the typing clash on lightning_module - haven't yet found a way to do it. Been tied up with work things last week so haven't had much chance to revisit

Copy link
Contributor

@otaj otaj left a comment

Choose a reason for hiding this comment

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

LGTM! I added a small commit resolving those last couple errors. We'll see how the IPU CI likes it 💪

@otaj otaj added this to the pl:1.7.x milestone Aug 2, 2022
@otaj
Copy link
Contributor

otaj commented Aug 2, 2022

Adding a bunch of asserts at correct places seemed to help, so now we're green with the tests again 🎉

@HalestormAI
Copy link
Contributor Author

Great! Thanks @otaj . I was trying to avoid all the asserts, but I guess that's the only way!

@HalestormAI
Copy link
Contributor Author

Should I update branch then create review? Do you prefer merge or rebase to line this up with master?

@otaj
Copy link
Contributor

otaj commented Aug 2, 2022

We are "merge" folks :) There's also a button "Update branch" (I don't know if you have it as well), which will merge master for you if there are no conflicts :)

@HalestormAI
Copy link
Contributor Author

Thanks @otaj - I've updated the branch and fixed the suggestion above. Once the CI completes I'll put it in for review

@otaj otaj marked this pull request as ready for review August 3, 2022 08:14
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

Solid

@mergify mergify bot added the ready PRs ready to be merged label Aug 3, 2022
@carmocca carmocca changed the title Tests/13445 ipu strategy typing annotations Fix typing annotations for the ipu strategy Aug 3, 2022
@carmocca carmocca modified the milestones: pl:1.7.x, pl:1.8 Aug 3, 2022
@rohitgr7 rohitgr7 requested review from carmocca and removed request for SeanNaren August 3, 2022 18:17
@rohitgr7 rohitgr7 enabled auto-merge (squash) August 3, 2022 19:27
@rohitgr7 rohitgr7 merged commit e293749 into Lightning-AI:master Aug 3, 2022
@HalestormAI HalestormAI deleted the tests/13445_ipu_strategy_typing_annotations branch August 4, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality community This PR is from the community pl Generic label for PyTorch Lightning package ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants