wandb logger 'global_step' affects other logger

[olineumann]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs? Not needed just a fix...
• Did you write any new necessary tests? Probably not necessary...
• If you made a notable change (that affects users), did you update the CHANGELOG?

Should I make an changelog update or will a maintainer do this?

What does this PR do?

Fixes #1485 .

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Shortly discussed in #1485 with @Borda

Did you have fun?

Yes a lot! :D

[awaelchli]

:)

[olineumann]

The reason why the tests are failling is because the tests are checking the actual 'log' call parameters. So the tests need to be updated right? Is there something I can do or should I just wait until someone fix this and merge it in 0.7.4? :)

I didn't go through all tests, but the one I checked all displayed the same:

E           AssertionError: Expected call: log({'acc': 1.0})
E           Actual call: log({'acc': 1.0}, step=None)

[Borda]

pls add change note

[olineumann]

pls add change note

Sorry currently I'm a little bit bussy and noticed it too late. Because 0.7.4 is over I added it to 0.7.5. Sorry...

[awaelchli]

I can fix the test if you are busy. Thanks for the pr.

[awaelchli]

actually no, I can't because of permissions haha. But the fix for the test is easy, just change line 19 to:

wandb.init().log.assert_called_once_with({'acc': 1.0}, step=None)

and line 23 to

wandb.init().log.assert_called_once_with({'acc': 1.0}, step=3)

[olineumann]

actually no, I can't because of permissions haha. But the fix for the test is easy, just change line 19 to:

wandb.init().log.assert_called_once_with({'acc': 1.0}, step=None)

and line 23 to

wandb.init().log.assert_called_once_with({'acc': 1.0}, step=3)

Ah okay... I didn't know I can affect the CI Pipeline. I thought only contibutors can do it. Sorry. I will take care of it tomorrow.

[olineumann]

Okay... It didn't let me rest so I quickly fixed it :D. On my machine, everything for wandb logger passed. Maybe one notice: I installed requirements.txt, requirements-extra.txt and tests/requirements.txt but some packages were missing yet. Did I missed one or should someone update requirements? Then someone should open an issue? I could update it in a few days after exams.

[codecov]

Codecov Report

Merging #1492 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1492   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          69      69           
  Lines        4133    4131    -2     
======================================
- Hits         3656    3654    -2     
  Misses        477     477           

[awaelchli]

thanks for fixing the test. the changelog entry should be moved to the "unreleased" section.

[olineumann]

thanks for fixing the test. the changelog entry should be moved to the "unreleased" section.

Done! Sorry about the mess... :/

[awaelchli]

No problem, thanks for fixing it so promptly.

[Borda]

LGTM 🦝

[mergify]

This pull request is now in conflict... :(

[Borda]

falling on test_lbfgs_cpu_model accuracy but I do not think it is connected...

[mergify]

This pull request is now in conflict... :(