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

Fix typehint for WandbLogger's log_model argument #18458

Merged

Conversation

SebastianGer
Copy link
Contributor

@SebastianGer SebastianGer commented Sep 1, 2023

What does this PR do?

Fixes #18370

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

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:

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

📚 Documentation preview 📚: https://pytorch-lightning--18458.org.readthedocs.build/en/18458/

…h should be the earliest point at which the error occurrs. Also removes config.yaml at the end of the test and changes to a temporary directory for each test.
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Sep 1, 2023
@SebastianGer
Copy link
Contributor Author

I added a test that fails before the bug fix and clears afterwards. I assume it should contain some kind of mock.patch for WandB, because that's included in the other WandbLogger tests. But I don't understand enough about this to know what to patch in where.

@awaelchli You seem to have worked on several commits of test_wandb.py, could you help with that?

I'm also not sure if the test should be moved somewhere else, but I guess that's something that can be looked at during the final review.

@awaelchli awaelchli added logger: wandb Weights & Biases community This PR is from the community bug Something isn't working labels Sep 4, 2023
@awaelchli awaelchli added this to the 2.0.x milestone Sep 4, 2023
@awaelchli awaelchli changed the title Bugfix/18370 wandblogger type hint Fix typehint for WandbLogger's log_model argument Sep 4, 2023
@awaelchli awaelchli marked this pull request as ready for review September 4, 2023 10:16
@mergify mergify bot added the ready PRs ready to be merged label Sep 4, 2023
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Merging #18458 (8c00623) into master (41ba7de) will decrease coverage by 34%.
Report is 2 commits behind head on master.
The diff coverage is 100%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #18458      +/-   ##
==========================================
- Coverage      85%      50%     -34%     
==========================================
  Files         427      419       -8     
  Lines       33263    33131     -132     
==========================================
- Hits        28119    16601   -11518     
- Misses       5144    16530   +11386     

tests/tests_pytorch/loggers/test_wandb.py Outdated Show resolved Hide resolved
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
@awaelchli awaelchli merged commit bff2e42 into Lightning-AI:master Sep 5, 2023
91 checks passed
Borda pushed a commit that referenced this pull request Sep 12, 2023
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: awaelchli <aedu.waelchli@gmail.com>
(cherry picked from commit bff2e42)
lantiga pushed a commit that referenced this pull request Sep 14, 2023
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: awaelchli <aedu.waelchli@gmail.com>
(cherry picked from commit bff2e42)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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.

WandBLogger: Can't set log_model from LightningCLI due to problem in type hint
4 participants