fix typing in pl/tuner/tuning.py #13616

donlapark · 2022-07-12T10:01:03Z

What does this PR do?

Fixes typing errors in pytorch_lightning/tuner/lr_finder.py in #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 🙃

* Refactor docker builds in CI * Reduce duplicate and merge two workflows * push: bool expression * Extend timeout for ipu builds * Update concurrency group * Define env for push to hub * Rename workflow * Fix bool expressions * Remove unnecessary trigger paths * Remove unused env var * Update job names * Trim timeout * rename Co-authored-by: Jirka <[email protected]>

rohitgr7 · 2022-07-12T10:17:29Z

src/pytorch_lightning/tuner/tuning.py

        early_stop_threshold: float = 4.0,
        update_attr: bool = False,
-    ) -> Optional[_LRFinder]:
+    ) -> Optional[Union[int, _LRFinder]]:


this was correct no?

Well, the tune is expected to return Optional[Union[int, _LRFinder]].

https://github.com/Lightning-AI/lightning/blob/6f5193244993750de2707b55cfe89e315310cc33/src/pytorch_lightning/trainer/trainer.py#L1010-L1018

mypy detects this and returns an assignment error.

yes, but lr_find returns result['lr_find'].

rohitgr7 · 2022-07-12T10:17:36Z

src/pytorch_lightning/tuner/tuning.py

        max_trials: int = 25,
        batch_arg_name: str = "batch_size",
-    ) -> Optional[int]:
+    ) -> Optional[Union[int, _LRFinder]]:


rohitgr7 · 2022-07-12T10:19:32Z

src/pytorch_lightning/tuner/tuning.py

        lr_find_kwargs = lr_find_kwargs or {}
        # return a dict instead of a tuple so BC is not broken if a new tuning procedure is added
-        result = {}
+        result: Dict[str, Any] = {}


Suggested change

result: Dict[str, Any] = {}

result: Dict[str, Union[int, _LRFinder]] = {}

Thanks! I think the Union has to be optional otherwise mypy will throw an error: Incompatible types in assignment (expression has type "Optional[int]", target has type "Union[int, _LRFinder]").
`

Suggested change

result: Dict[str, Any] = {}

result: Dict[str, Optional[Union[int, _LRFinder]]] = {}

…#13538) Co-authored-by: Akihiro Nitta <[email protected]>

…3494) Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]>

* Enable dependabot on GHA * Update comment * Update PR limit Co-authored-by: Jirka Borovec <[email protected]>

fix typo

Update labeler

Remove redundant test

* CI: hotfix gatekeeper * no min * min 1

…13600) Co-authored-by: Carlos Mocholí <[email protected]>

carmocca

I would explore using a TypedDict instead:

from typing_extensions import TypedDict

class _TunerResult(TypedDict):
    lr_find: Optional[_LRFinder]
    scale_batch_size: Optional[int]

Which should clarify the ambiguity and improve the type checks

Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 2 to 3. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v2...v3) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 1 to 2. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@v1...v2) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…hSamplerWrapper.batch_indices` (#13565) * Removed the deprecated method * Removed deprecated IndexBatchSamplerWrapper.batch_indices * Update src/pytorch_lightning/CHANGELOG.md * Missed code Co-authored-by: Akihiro Nitta <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]>

for more information, see https://pre-commit.ci

donlapark · 2022-07-13T07:42:19Z

Thanks. I'll close this PR and incorporate the suggestions in the next one.

fix typing in pl/tuner/tuning.py

7c4b7df

donlapark requested review from Borda, SeanNaren, SkafteNicki, awaelchli, carmocca, justusschock, kaushikb11, rohitgr7, tchaton and williamFalcon as code owners July 12, 2022 10:01

rohitgr7 reviewed Jul 12, 2022

View reviewed changes

rohitgr7 added this to the pl:1.7 milestone Jul 12, 2022

otaj mentioned this pull request Jul 12, 2022

Improve typing coverage (4/n) #13445

Closed

52 tasks

rohitgr7 added tuner code quality community This PR is from the community labels Jul 12, 2022

jxtngx and others added 8 commits July 12, 2022 11:11

Fix mypy errors attributed to pytorch_lightning.loggers.csv_logs.py (…

0a6dc52

…#13538) Co-authored-by: Akihiro Nitta <[email protected]>

Fix mypy errors attributed to pytorch_lightning.loggers.base.py (#1…

57f5e18

…3494) Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]>

CI: Enable dependabot for GitHub Actions (#13589)

e287f22

* Enable dependabot on GHA * Update comment * Update PR limit Co-authored-by: Jirka Borovec <[email protected]>

Fix default value for enable_progress_bar in docs (#13584)

b9ceefb

fix typo

CI: Update labeler bot (#13624)

855a331

Update labeler

Remove redundant GPU test (#13623)

dba65be

Remove redundant test

CI: hotfix gatekeeper (#13606)

9468866

* CI: hotfix gatekeeper * no min * min 1

Remove add_to_queue and remove_from_queue from LightningModule (#…

e034cd3

…13600) Co-authored-by: Carlos Mocholí <[email protected]>

carmocca reviewed Jul 12, 2022

View reviewed changes

dependabot bot added 3 commits July 12, 2022 16:13

Bump codecov/codecov-action from 1 to 3 (#13620)

01209bd

samz5320 and others added 4 commits July 13, 2022 00:53

change typing of tune() to TypedDict

71aefc5

change typing of tune() to TypedDict

0fbd69b

change typing of tune() to TypedDict

50dd785

donlapark requested review from akihironitta and edenlightning as code owners July 13, 2022 07:34

[pre-commit.ci] auto fixes from pre-commit.com hooks

d5cfa4a

for more information, see https://pre-commit.ci

donlapark closed this Jul 13, 2022

donlapark deleted the fix_typing_pl_tuner_tuning branch July 13, 2022 07:47

donlapark mentioned this pull request Jul 13, 2022

Change the return type of tune() in trainer.py to TypedDict #13631

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix typing in pl/tuner/tuning.py #13616

fix typing in pl/tuner/tuning.py #13616

Uh oh!

donlapark commented Jul 12, 2022

Uh oh!

rohitgr7 Jul 12, 2022

Uh oh!

donlapark Jul 12, 2022

Uh oh!

rohitgr7 Jul 12, 2022

Uh oh!

rohitgr7 Jul 12, 2022

Uh oh!

rohitgr7 Jul 12, 2022

Uh oh!

donlapark Jul 12, 2022

Uh oh!

donlapark Jul 12, 2022

Uh oh!

carmocca left a comment

Uh oh!

donlapark commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

	result: Dict[str, Any] = {}
	result: Dict[str, Union[int, _LRFinder]] = {}

	result: Dict[str, Any] = {}
	result: Dict[str, Optional[Union[int, _LRFinder]]] = {}

fix typing in pl/tuner/tuning.py #13616

fix typing in pl/tuner/tuning.py #13616

Uh oh!

Conversation

donlapark commented Jul 12, 2022

What does this PR do?

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

Before submitting

PR review

Did you have fun?

Uh oh!

rohitgr7 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

donlapark Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

rohitgr7 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

rohitgr7 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

rohitgr7 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

donlapark Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

donlapark Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

carmocca left a comment

Choose a reason for hiding this comment

Uh oh!

donlapark commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants