-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(wandb): support distributed modes #11650
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
Conversation
|
Amazing! I tried and it works great so far. Couldn't push to your branch for some reason. I suggest to update the tests: diff --git a/tests/loggers/test_wandb.py b/tests/loggers/test_wandb.py
index 85b20c562..d347d32d1 100644
--- a/tests/loggers/test_wandb.py
+++ b/tests/loggers/test_wandb.py
@@ -22,6 +22,7 @@ from pytorch_lightning import Trainer
from pytorch_lightning.loggers import WandbLogger
from pytorch_lightning.utilities.exceptions import MisconfigurationException
from tests.helpers import BoringModel
+from tests.helpers.utils import no_warning_call
@mock.patch("pytorch_lightning.loggers.wandb.wandb")
@@ -57,7 +58,7 @@ def test_wandb_logger_init(wandb):
# verify default resume value
assert logger._wandb_init["resume"] == "allow"
- with pytest.warns(UserWarning, match="There is a wandb run already in progress"):
+ with no_warning_call(UserWarning, match="There is a wandb run already in progress"):
_ = logger.experiment
logger.log_metrics({"acc": 1.0}, step=3)
@@ -124,8 +125,10 @@ def test_wandb_pickle(wandb, tmpdir):
def test_wandb_logger_dirs_creation(wandb, tmpdir):
"""Test that the logger creates the folders and files in the right place."""
logger = WandbLogger(save_dir=str(tmpdir), offline=True)
- assert logger.version is None
- assert logger.name is None
+
+ # the wandb Run gets initialized on instantiation, version and name should be available
+ assert logger.version is not None
+ assert logger.name is not None
# mock return values of experiment
wandb.run = None |
Co-authored-by: Adrian Wälchli <[email protected]>
justusschock
left a comment
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 in general, some very minor nit :)
|
Confused about the pre-commit error. |
pre-commit errors seem correct since there is an import that isn't used. |
|
I was not able to understand the problem with Also I tried using |
Co-authored-by: Rohit Gupta <[email protected]>
|
hey @borisdayma here is the updated code with monkeypatch you can use: https://pastebin.com/hvkyGxqc |
|
Many thanks @rohitgr7 |
What does this PR do?
Fixes #11602
Improves support of distributed mode.
For now it also requires calling
wandb.require("service")in training script.Does your PR introduce any breaking changes? If yes, please list them.
The run is created earlier than it used to be as it happens when we instantiate
WandbLoggerwhile it used to be created when first logging metric, hyper-parameters or logging any other supported method.Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @awaelchli @kptkin @raubitsj who are also working on this PR
cc @morganmcg1 @scottire @manangoel99