-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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(wandb): allow use of sweeps #1512
Conversation
overwrite run config parameters due to precision error fix Lightning-AI#1290
Codecov Report
@@ Coverage Diff @@
## master #1512 +/- ##
======================================
Coverage 88% 88%
======================================
Files 68 68
Lines 3956 3956
======================================
Hits 3501 3501
Misses 455 455 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add a test for it
The test has been updated but the way it's set up, we only see that the function has been called -> see here This is due to the fact that wandb uses context and it does not work well with pytest (there was some errors with some platforms tested such as windows). Similar behavior happens with other loggers which also use mocks and verify only that the correct function has been called. Adding another test overwriting a previous value would only check the same function has been called (so no more than current test scope). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐰
This pull request is now in conflict... :( |
Before submitting
not applicable
What does this PR do?
Fixes #1290 .
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.
Did you have fun?
Make sure you had fun coding 🙃