Skip to content

Conversation

@gautierdag
Copy link
Contributor

What does this PR do?

This PR fixes mypy errors for the pytorch_lightning/loggers/wandb.py file.

Note, I had to loosen the typing in loggers.py to allow an Optional[str] return for both name and version functions of the Logger class.

There are a couple additional types that could be imported from wandb for stricter checks, for instance the ImageDataOrPathType type could be used to check valid image data. However, I resorted to keeping it Any for now.

Fixes pytorch_lightning/loggers/wandb.py from #13445

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

No

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 🙃

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.

Hi, thanks for the PR, I left one comment in the code. It also seems one additional test gets broken by your change related to CLI (utilities/test_cli.py::test_wandb_logger_init_args). However, I'm not too familiar with CLI tests, would someone else mind taking a look at that test please? cc @carmocca as he helped land the particular failing test

@carmocca
Copy link
Contributor

carmocca commented Jul 1, 2022

However, I'm not too familiar with CLI tests, would someone else mind taking a look at that test please? cc @carmocca as he helped land the particular failing test

I'm actually not sure about what's the problem. I see:

E               ValueError: Value "Namespace(class_path='pytorch_lightning.loggers.WandbLogger', init_args=Namespace(notes='wandb', save_dir='wandb'))" does not validate against any of the types in typing.Union[pytorch_lightning.loggers.logger.Logger, typing.Iterable[pytorch_lightning.loggers.logger.Logger], bool]:
E                 - Problem with given class_path "pytorch_lightning.loggers.WandbLogger":
E                   - Component not supported: Call(func=Attribute(value=Attribute(value=Name(id='self', ctx=Load()), attr='_wandb_init', ctx=Load()), attr='update', ctx=Load()), args=[], keywords=[keyword(arg=None, value=Name(id='kwargs', ctx=Load()))])

Perhaps you know @mauvilsa?

@gautierdag
Copy link
Contributor Author

gautierdag commented Jul 1, 2022

I'm actually not sure about what's the problem. I see:

E               ValueError: Value "Namespace(class_path='pytorch_lightning.loggers.WandbLogger', init_args=Namespace(notes='wandb', save_dir='wandb'))" does not validate against any of the types in typing.Union[pytorch_lightning.loggers.logger.Logger, typing.Iterable[pytorch_lightning.loggers.logger.Logger], bool]:
E                 - Problem with given class_path "pytorch_lightning.loggers.WandbLogger":
E                   - Component not supported: Call(func=Attribute(value=Attribute(value=Name(id='self', ctx=Load()), attr='_wandb_init', ctx=Load()), attr='update', ctx=Load()), args=[], keywords=[keyword(arg=None, value=Name(id='kwargs', ctx=Load()))])

Perhaps you know @mauvilsa?

Seems like the error is due to the "notes" key in the actual test 😕

@pytest.mark.skipif(not _WANDB_AVAILABLE, reason="wandb is required")
def test_wandb_logger_init_args():
    _test_logger_init_args("WandbLogger", {"save_dir": "wandb", "notes": "wandb"})
'Configuration check failed :: No action for destination key "notes" to check its value.'
 - Expected a List but got "Namespace(class_path='pytorch_lightning.loggers.WandbLogger', init_args=Namespace(name=None, save_dir='wandb', offline=False, id=None, anonymous=None, version=None, project=None, log_model=False, experiment=None, prefix='', agg_key_funcs=None, agg_default_func=None, notes='wandb'))"
  - Expected a <class 'bool'> but got "Namespace(class_path='pytorch_lightning.loggers.WandbLogger', init_args=Namespace(name=None, save_dir='wandb', offline=False, id=None, anonymous=None, version=None, project=None, log_model=False, experiment=None, prefix='', agg_key_funcs=None, agg_default_func=None, notes='wandb'))"

Changing "notes" to "name" like in the other logger tests fixes the issue. However I'm confused at how this test was even passing in the first place..

@otaj otaj added this to the pl:1.7 milestone Jul 8, 2022
@otaj otaj added code quality community This PR is from the community labels Jul 8, 2022
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Jul 15, 2022
auto-merge was automatically disabled July 19, 2022 09:41

Head branch was pushed to by a user without write access

@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Jul 19, 2022
@awaelchli awaelchli enabled auto-merge (squash) July 19, 2022 10:13
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 20, 2022
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #13483 (7b2708e) into master (e3b29cb) will decrease coverage by 10%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #13483     +/-   ##
=========================================
- Coverage      86%      76%    -10%     
=========================================
  Files         327      327             
  Lines       25485    25486      +1     
=========================================
- Hits        21893    19446   -2447     
- Misses       3592     6040   +2448     

@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Jul 21, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Jul 21, 2022
@awaelchli awaelchli merged commit 0e53128 into Lightning-AI:master Jul 21, 2022
justusschock pushed a commit that referenced this pull request Jul 21, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
justusschock added a commit that referenced this pull request Jul 25, 2022
* Rename GPUAccelerator to CUDAAccelerator

* Add back GPUAccelerator and deprecate it

* Remove temporary registration

* accelerator connector reroute

* accelerator_connector tests

* update enums

* lite support + tests

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

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

* typo

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

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

* move "gpu" support up before actual accelerator flag checks

* Stupid arguments

* fix tests

* change exception type

* fix registry test

* pre-commit

* CI: debug HPU flow (#13419)

* Update the hpu-tests.yml to pull docker from vault
* fire & sudo
* habana-gaudi-hpus
* Check the driver status on gaudi server (#13718)

Co-authored-by: arao <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Akarsha Rao <[email protected]>

* Update typing-extensions requirement from <4.2.1,>=4.0.0 to >=4.0.0,<4.3.1 in /requirements (#13529)

Update typing-extensions requirement in /requirements

Updates the requirements on [typing-extensions](https://github.com/python/typing_extensions) to permit the latest version.
- [Release notes](https://github.com/python/typing_extensions/releases)
- [Changelog](https://github.com/python/typing_extensions/blob/main/CHANGELOG.md)
- [Commits](python/typing_extensions@4.0.0...4.3.0)

---
updated-dependencies:
- dependency-name: typing-extensions
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [pre-commit.ci] pre-commit suggestions (#13540)

updates:
- [github.com/psf/black: 22.3.0 → 22.6.0](psf/black@22.3.0...22.6.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* [FIX] Native FSDP precision + tests (#12985)

* Simplify fetching's loader types (#13111)

* Include app templates to the lightning and app packages (#13731)

* Include app templates to the package

Co-authored-by: mansy <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>

* Fix mypy typing errors in pytorch_lightning/callbacks/model_checkpoint.py (#13617)

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

* Fix typos initialize in docs (#13557)


Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>

* Fix main progress bar counter when `val_check_interval=int` and `check_val_every_n_epoch=None` (#12832)

* Fix mypy errors attributed to `pytorch_lightning.loggers.tensorboard.py` (#13688)

Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: Carlos Mocholí <[email protected]>

* Fix mypy errors attributed to `pytorch_lightning.loggers.mlflow` (#13691)

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: otaj <[email protected]>

* fix mypy errors for loggers/wandb.py (#13483)


Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>

* Fix gatekeeper minimum check (#13769)

* changelog

* changelog

* fix order

* move up again

* add missing test

Co-authored-by: rohitgr7 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: arao <[email protected]>
Co-authored-by: Akarsha Rao <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sean Naren <[email protected]>
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Mansy <[email protected]>
Co-authored-by: mansy <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Lee Jungwon <[email protected]>
Co-authored-by: Nathaniel D'Amours <[email protected]>
Co-authored-by: Justin Goheen <[email protected]>
Co-authored-by: otaj <[email protected]>
Co-authored-by: Gautier Dagan <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
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 logger: wandb Weights & Biases 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.

8 participants