Skip to content

fix(wandb): prevent WandbLogger from dropping values#5931

Merged
awaelchli merged 26 commits intoLightning-AI:masterfrom
borisdayma:feat-wandb_x
Feb 27, 2021
Merged

fix(wandb): prevent WandbLogger from dropping values#5931
awaelchli merged 26 commits intoLightning-AI:masterfrom
borisdayma:feat-wandb_x

Conversation

@borisdayma
Copy link
Copy Markdown
Contributor

@borisdayma borisdayma commented Feb 12, 2021 •
edited by Borda
Loading

What does this PR do?

This PR simplifies use of WandbLogger.

PR #5351 introduced a workaround for dealing with non monotonous steps.
The consequence was that some logged values would get dropped. It was suggested to the user to use sync_step=False, forcing the user to rerun his experiment.

The new behavior:

  • the step is logged as train/step
  • if using latest wandb version, it will automatically be set as x-axis by default
  • no need to use wandb.log(items, commit=False) to prevent step from auto-increasing

This will prevent any value from being dropped.

Should step not be increasing for any reason, users will be able to select a different x-axis.

There are no breaking changes because the current version (with sync_step) had not been released yet.

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read 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
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Feb 12, 2021 •
edited
Loading

Hello @borisdayma! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-27 01:26:56 UTC

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2021 •
edited
Loading

Codecov Report

Merging #5931 (160c16b) into master (ee5032a) will decrease coverage by 2%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5931    +/-   ##
=======================================
- Coverage      93%     91%    -2%     
=======================================
  Files         159     159            
  Lines       11384   11380     -4     
=======================================
- Hits        10628   10358   -270     
- Misses        756    1022   +266

@borisdayma borisdayma changed the title feat(wandb): simplify WandbLogger with new API fix(wandb): prevent WandbLogger from dropping values Feb 12, 2021
This was referenced Feb 12, 2021
Closed
Closed
@raubitsj raubitsj mentioned this pull request Feb 16, 2021
Merged
26 tasks
@awaelchli awaelchli added bug Something isn't working logger Related to the Loggers priority: 1 Medium priority task labels Feb 17, 2021
@awaelchli awaelchli added this to the 1.2.x milestone Feb 17, 2021
@borisdayma borisdayma marked this pull request as ready for review February 17, 2021 17:47
@borisdayma
Copy link
Copy Markdown
Contributor Author

borisdayma commented Feb 17, 2021

It's now ready for review.
We won't see any changes in the UI as of now (ie default x-axis won't be auto-set) but it will automatically appear with the next release of wandb.

It would be great to have it merged before people have a chance to use the "sync_step" functionality (which is in master but not released yet).

Let me know if you have any comments. I'll fix the simple merge issues.

@Borda
Copy link
Copy Markdown
Collaborator

Borda commented Feb 17, 2021

@borisdayma mind resolve conflicts?

@mergify mergify bot removed the has conflicts label Feb 25, 2021
@awaelchli awaelchli enabled auto-merge (squash) February 27, 2021 01:23
@awaelchli awaelchli mentioned this pull request Feb 27, 2021
Merged
11 tasks
@awaelchli awaelchli merged commit 40d5a9d into Lightning-AI:master Feb 27, 2021
kaushikb11 pushed a commit to kaushikb11/pytorch-lightning that referenced this pull request Mar 2, 2021
@borisdayma @carmocca
@tchaton @awaelchli @kaushikb11
fix(wandb): prevent WandbLogger from dropping values (Lightning-AI#5931)
2ca2d52 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
kaushikb11 pushed a commit to kaushikb11/pytorch-lightning that referenced this pull request Mar 2, 2021
@borisdayma @carmocca
@tchaton @awaelchli @kaushikb11
fix(wandb): prevent WandbLogger from dropping values (Lightning-AI#5931)
3e5584e 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
lexierule pushed a commit that referenced this pull request Mar 5, 2021
@borisdayma @carmocca
@tchaton @awaelchli @lexierule
fix(wandb): prevent WandbLogger from dropping values (#5931)
5abfd2c 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@awaelchli awaelchli mentioned this pull request Mar 7, 2021
Closed
This was referenced Mar 8, 2021
Closed
Closed
Closed
Closed
This was referenced Mar 12, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@raubitsj raubitsj mentioned this pull request Mar 14, 2021
Merged
1 task
@dependabot dependabot bot mentioned this pull request Mar 15, 2021
Closed
@vpeopleonatank vpeopleonatank mentioned this pull request Apr 22, 2021
Closed
@carmocca carmocca mentioned this pull request Aug 5, 2021
Merged
11 tasks
@anmol-wiai
Copy link
Copy Markdown

anmol-wiai commented Aug 26, 2022

Hi,

Note: When logging manually through `wandb.log` or `trainer.logger.experiment.log`,
make sure to use `commit=False` so the logging step does not increase.

Is this note relevant after this PR?

@slumericanBx slumericanBx mentioned this pull request Oct 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaton tchaton tchaton left review comments

@Borda Borda Borda approved these changes

@justusschock justusschock justusschock approved these changes

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

+2 more reviewers

@awaelchli awaelchli awaelchli approved these changes

@carmocca carmocca carmocca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working logger Related to the Loggers priority: 1 Medium priority task ready to be merged PRs ready to be merged

Projects

None yet

Milestone

pl:1.2.x

Development

Successfully merging this pull request may close these issues.

8 participants

@borisdayma @pep8speaks @Borda @carmocca @anmol-wiai @awaelchli @tchaton @justusschock