ClusterEnvironment interface and improvements

[awaelchli]

🚀 Feature

In #5915 we started discussing what a good interface for the cluster environment could be and where it is headed in the future.
We should iterate on these ideas.

Some ideas / issues / refactors to discuss:

• there is a small amount of code duplication.

• Review Add LSF support #5102 and determine if interface needs changes

• Add docs for all environments

• Teardown logic: cluster environments that set environment variables (globally) should also be responsible for cleaning them up ([fix] Add a cluster environment teardown to clean up environment state #6942)

• Convert methods to proper setters/getters (Rename "master" methods to "main" in ClusterEnvironment plugins #10103)

• Consider renaming "creates_children". It is not clear if it means the environment within Lightning or the actual cluster. cc @Borda @justusschock @awaelchli @akihironitta @rohitgr7 @tchaton @ananthsub Remove deprecated method ClusterEnvironment.creates_children #10339

• Move left over slurm logic in slurm connector to the slurm environment (refactor slurm_job_id #10622 Deprecate trainer.slurm_job_id #10615)

• A detect method:

# returns true if we can detect the cluster environment.
def detect():
    if ...
        return True

[leezu]

Trainer has a SLURM specific property. Should it be deprecated as part of the ClusterEnvironment interface improvements?

https://github.com/PyTorchLightning/pytorch-lightning/blob/8c0ea92af237542f5b36ae684543f871da829379/pytorch_lightning/trainer/properties.py#L176-L189

[awaelchli]

Hi, good observation. I would say the logic inside of it can and should definitely move into the cluster plugin. I would still keep the property as an easy way to access the cluster environment IF that is something users actually access (I have no idea).

[ananthsub]

Consider renaming "creates_children". It is not clear if it means the environment within Lightning or the actual cluster. cc @ananthsub

Since it came up in #9389 - we need a consistent value here during the course of the job. relying on the mutable os.environ for instance can lead to surprising side effects. a naming nit would be to to rename this as created_children to be ultra clear that this either happened or didn't, as opposed to a continuous/ongoing behavior.

[awaelchli]

@ananthsub

a naming nit would be to to rename this as created_children to be ultra clear that this either happened or didn't, as opposed to a continuous/ongoing behavior.

I propose "creates_processes_externally" as a clearer name. #10106
Suggestions for better names welcome :)

[carmocca]

@awaelchli is this done? can we close?

[awaelchli]

@carmocca there is one item open

Convert methods to proper setters/getters

It came up in reviews that people were wondering why the methods are not properties. They are not properties because the plugins are meant to be customized by overriding them and in Lightning we have the strict rule to avoid decorators when it comes to overriding hooks or similar. We can attempt to make a decision or just close the issue until someone brings this up again. The reason why nobody has done it so far is probably because integrating backward compatibility is absolutely insane on this one.

[carmocca]

If you don't mind, can you open a separate issue for just that so we close this one?

because integrating backward compatibility is absolutely insane on this one.

In the new issue, can you broadly describe what would be the pattern used to keep backwards compatibility?

[awaelchli]

Follow up #12025