fix Neptune logger creating multiple experiments when gpus > 1

[psinger]

Potential fix to #3255

[awaelchli]

kindly asking @jakubczakon for review. Can we delay the creation of the experiment or does i thave to happen in init?

[pitercl]

Hi @psinger, could you please paste a snippet that reproduces this issue? From what I remember, the idea was to have NeptuneLogger initiated before any forking happens. Then, when the logger is pickled, the experiment that was created in __init__ can be reused in all children processes.

[psinger]

@pitercl I am creating the logger before initializing the Trainer.

logger = NeptuneLogger()
trainer = Trainer(logger=logger,  distributed_backend = "ddp")
model = Model()
trainer.fit(model)

[pitercl]

Thanks @psinger. I just checked and you're right - it was working as I described up to PL 0.7.6 and from 0.8.1 something's changed and the results are as you say. I'll need some time to understand what has changed and how to approach this.

[awaelchli]

@pitercl @psinger I can explain. distributed_backend = "ddp" is special in that it launches your script multiple times in a new subprocess. This means init is called several times, but the way we designed loggers is that the logger.experiment only returns the true logger object on rank == 0. On all other ranks, it returns a dummy object.
So what we have to make sure is that we don't create any files in the init, because that would run on all ranks.

[pitercl]

Hi!

@awaelchli Thanks for the explanation - it helped a lot in understanding what's going on.

@psinger Your idea for the fix looks good to me 👍

As for the tests that stopped passing, I had 2 goals with them:

1. I wanted to check if online/offline modes do what they're supposed to
2. Make sure that the experiment was created in __init__. This was important in the earlier version of PL, where the pickled logger was passed to child processes and used there (I didn't want to have a new experiment in each child process). I don't think it matters now, after the @rank_zero_experiment change.

So, I'd propose something along the lines of:

@patch('pytorch_lightning.loggers.neptune.neptune')
def test_neptune_online(neptune):
    logger = NeptuneLogger(api_key='test', project_name='project')

    experiment = logger.experiment  # force the actual creation of an experiment object

    assert experiment == neptune.Session.with_default_backend().get_project().create_experiment()
    assert logger.name == experiment.name
    assert logger.version == experiment.id


@patch('pytorch_lightning.loggers.neptune.neptune')
def test_neptune_offline(neptune):
    logger = NeptuneLogger(offline_mode=True)

    experiment = logger.experiment  # force the actual creation of an experiment object

    neptune.Session.assert_called_once_with(backend=neptune.OfflineBackend())
    assert experiment == neptune.Session().get_project().create_experiment()```

[mergify]

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

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

[stale]

This pull request is going to be closed. Please feel free to reopen it create a new from the actual master.

[Borda]

@psinger can we finish this one?

[psinger]

From my perspective yes

[Parskatt]

@psinger
Status on this? Doesn't seem to be on master?

Also I don't understand your fix? It seems you always set the experiment to None? Doesnt this remove the experiment even for rank_zero?
Why not just put a rank_zero_only decorator on _create_or_get_experiment ?

[psinger]

@Parskatt I cannot give you a status on this. My fix solves the issue though.

[awaelchli]

@psinger I rebased the branch and updated the tests so that they pass with the change you made in neptune.
When doing that, I saw this comment in the test. I'm not sure what this is about. I see no evidence that we are forced to initialize the neptune experiment at init. How do you see it?

[awaelchli]

Let's finalize this PR, it has waited long enough :)

[codecov]

Codecov Report

Merging #3256 (ed89104) into master (6ab5417) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #3256   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         135     135           
  Lines       10005   10005           
======================================
+ Hits         9339    9340    +1     
+ Misses        666     665    -1