ModelCheckpoint wipes out current directory

[neggert]

@williamFalcon I think this is what you were seeing in #389. If we let the Trainer create the default ModelCheckpoint callback and don't use TestTubeLogger, the prefix ends up being set to the current directory. Then, when ModelCheckpoint tries to clean up previous checkpoints, it wipes out everything in the current directory.

Relevant bits of code:

• default_save_path set to os.getcwd(): https://github.com/williamFalcon/pytorch-lightning/blob/master/pytorch_lightning/trainer/trainer.py#L151
• ModelCheckpoint falls back to default_save_path: https://github.com/williamFalcon/pytorch-lightning/blob/master/pytorch_lightning/trainer/trainer.py#L280
  ( ModelCheckpoint blows away pre-existing files in checkpoint directory: https://github.com/williamFalcon/pytorch-lightning/blob/master/pytorch_lightning/callbacks/pt_callbacks.py#L216

The most obvious fix is to provide a better default checkpoint prefix, but there would still be a lurking footgun for a user who sets default_save_path incorrectly. Should we maybe insist that the checkpoint directory not exist before training starts, or that it be empty?

[williamFalcon]

most of the time it makes sense to have the default checkpoint inside the experiment file. why don't we do that with mlflowLogger? mlflow_exp/checkpoints

[neggert]

MLFlow doesn't always have an experiment file locally. You can set it up to do all logging to a centralized server. Presumably other loggers will have similar issues. I'm not sure it makes sense to tightly couple checkpoint saving to the logger as a general principle.

How about this proposal:

• Provide a default value of default_save_path="./checkpoints"
• Add a new (required) name property to LightningLoggerBase and make the existing version property required
• Have the default ModelCheckpoint save to os.path.join(default_save_path, logger.name, logger.version) if a logger is defined, otherwise default_save_path.
• Add a warning in ModelCheckpoint if filepath already exists and has files in it.

I think this should prevent accidentally deleting files unless the user manually sets default_save_path=os.getcwd() and ignores the warning. I'm happy to do a PR if this sounds good.

[neggert]

@williamFalcon Thoughts on the above proposal? If this is what we want to do, you can assign to me.

[williamFalcon]

yeah, i like that. But default logging should be os.path.join(default_save_path, logger.name, logger.version, '/checkpoints')

Isn't default_save_path also used for the logger path? and also SLURM checkpointing, etc...? it shouldn't go to ./checkpoints specifically

[neggert]

Yeah, I think with the other changes, leaving default_save_path=os.getcwd() is fine. I was thinking about the case where there's no logger, but the default checkpoint, but I think we can just fall back to checkpoints in that case.

[neggert]

I notice that for test tube, we're pre-pending version_ before the version. Is that needed? If so, I maybe we can do it in the loggers version property.

[williamFalcon]

@neggert i made the fix to the PR on version_. See comments there

[philippslang]

Thanks for this fix @neggert. I just tested this with a remote databricks workspace, e.g.

mlf_logger = MLFlowLogger(
            experiment_name="/Users/xxxx/xxx",
            tracking_uri="databricks"
        )

Now the experiment name is exposed as the name of the logger which in turn is used to create the checkpoint directory. Long story short it tries to create a directory Users under the root partition. Now this a loud error and still better than cleaning out the current directory 😏 but I'm still looking for a fix. Any pointers on how to address this and I can take a stab at it (also @williamFalcon ). I'll create an issue.

[neggert]

Why are you setting the experiment name to a file path? Is this something that's required with databricks?

You might want to take a look at the default_save_path argument to Trainer. The checkpoints will be saved in the result of

                ckpt_path = os.path.join(
                    self.default_save_path,
                    self.logger.name,
                    f'version_{self.logger.version}',
                    "checkpoints"
                )

All of those parameters except the final "checkpoints" are adjustable, so you should be able to get the checkpoints to save wherever you want.

[williamFalcon]

let’s resolve this today to make the release for tomorrow