Repository: Lightning-AI/pytorch-lightning
Pull request: Report leaking environment variables in tests
## What does this PR do?

Currently Lightning "pollutes" the environment with variables. To some degree, this is necessary for example for multiprocessing etc. but unfortunately the variables don't get cleaned up properly, and it may not be so obvious where and what to clean up.

We currently have this fixture which will restore the environment after each test:

```python
@pytest.fixture(scope="function", autouse=True)
def restore_env_variables():
    env_backup = os.environ.copy()
    yield
    os.environ.clear()
    os.environ.update(env_backup)
```

~~This PR changes this into an assertion and each test needs to leave the environment unchanged. 
For now, this fixture serves the purpose to identify which tests are leaking. After we fix all the issues, we can include this fixture for every test.
In order to solve this, we need proper teardown logic for Trainer, Accelerator, Plugins, Logger etc.
Fixes #5757~~

This PR reports any new leaking environment variables.

## Before submitting
- [ ] Was this discussed/approved via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] Did you make sure your PR does only one thing, instead of bundling different changes together?
- [x] Did you make sure to update the documentation with your changes? (if necessary)
- [x] Did you write any new necessary tests? (not for typos and docs)
- [x] Did you verify new and existing tests pass locally with your changes?
- [x] Did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- For CHANGELOG separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

 - [x] Is this pull request ready for review? (if not, please submit in draft mode)
 - [x] Check that all items from **Before submitting** are resolved
 - [x] Make sure the title is self-explanatory and the description concisely explains the PR
 - [x] Add labels and milestones (and optionally projects) to the PR so it can be classified
 - [x] **Check that target branch and milestone match!**


## Did you have fun?
Make sure you had fun coding 🙃


Problem statement:
strange behavior with tests in PL: tests influence each other
## 🐛 Bug

I observed that some tests under tests/trainer have very strange behavior. The order in which the tests are executed seems to matter and they are influencing each other!

### To Reproduce

1. Checkout` release/1.2-dev` branch
2. Remove the `predict()` method in one of the accelerators (GPUAccelerator is enough)
3. Run `py.test -v tests/trainer`

You will see that a ton of unrelated tests fail, but **none** of them report "Accelerator has not attribute predict". Which should be the only case why a test would fail. But be prepared, it gets funnier. 

4. Run only a single test out of all these that failed, for example: 
    `py.test -v tests\trainer\optimization\test_optimizers.py::test_lr_scheduler_strict`

And it passes! What?


### Expected behavior

Tests should not have an influence on each other.

### Environment

* CUDA:
        - GPU:
                - GeForce GTX 1070
        - available:         True
        - version:           10.2
* Packages:
        - numpy:             1.18.1
        - pyTorch_debug:     True
        - pyTorch_version:   1.7.0
        - pytorch-lightning: 1.2.0dev
        - tqdm:              4.55.1
* System:
        - OS:                Windows
        - architecture:
                - 64bit
                - WindowsPE
        - processor:         AMD64 Family 23 Model 113 Stepping 0, AuthenticAMD
        - python:            3.7.7
        - version:           10.0.19041


Also on my linux node, same behavior

### Additional context

Hints from issue discussion:
Hey @awaelchli,

Could it be linked to the new predict function. There is a bug which i badly solved there: https://github.com/PyTorchLightning/pytorch-lightning/pull/5752/files#diff-667a2513c158c2b01d73b479e4dea75587d8531f9cd286891d34570a2fd145dcR427

Best,
T.C

Commit message: Add RANK from DeepSpeed

Patch to review:
diff --git a/tests/conftest.py b/tests/conftest.py
index 9efe445ce812f..6c111f606516d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -47,9 +47,26 @@ def restore_env_variables():
     """Ensures that environment variables set during the test do not leak out."""
     env_backup = os.environ.copy()
     yield
+    leaked_vars = os.environ.keys() - env_backup.keys()
     # restore environment as it was before running the test
     os.environ.clear()
     os.environ.update(env_backup)
+    # these are currently known leakers - ideally these would not be allowed
+    allowlist = {
+        "CUDA_DEVICE_ORDER",
+        "LOCAL_RANK",
+        "NODE_RANK",
+        "WORLD_SIZE",
+        "MASTER_ADDR",
+        "MASTER_PORT",
+        "PL_GLOBAL_SEED",
+        "PL_SEED_WORKERS",
+        "WANDB_MODE",
+        "HOROVOD_FUSION_THRESHOLD",
+        "RANK",  # set by DeepSpeed
+    }
+    leaked_vars.difference_update(allowlist)
+    assert not leaked_vars, f"test is leaking environment variable(s): {set(leaked_vars)}"
 
 
 @pytest.fixture(scope="function", autouse=True)
diff --git a/tests/loggers/test_tensorboard.py b/tests/loggers/test_tensorboard.py
index 5e82c79463598..02a809aa2ab30 100644
--- a/tests/loggers/test_tensorboard.py
+++ b/tests/loggers/test_tensorboard.py
@@ -120,6 +120,7 @@ def test_tensorboard_no_name(tmpdir, name):
     assert os.listdir(tmpdir / "version_0")
 
 
+@mock.patch.dict(os.environ, {})
 def test_tensorboard_log_sub_dir(tmpdir):
     class TestLogger(TensorBoardLogger):
         # for reproducibility


Code review comments:
diff_hunk: @@ -47,9 +47,26 @@ def restore_env_variables():
     """Ensures that environment variables set during the test do not leak out."""
     env_backup = os.environ.copy()
     yield
+    leaked_vars = os.environ.keys() - env_backup.keys()
     # restore environment as it was before running the test
     os.environ.clear()
     os.environ.update(env_backup)
+    # these are currently known leakers - ideally these would not be allowed
+    allowlist = {
+        "CUDA_DEVICE_ORDER",
+        "LOCAL_RANK",
+        "NODE_RANK",
+        "WORLD_SIZE",
+        "MASTER_ADDR",
+        "MASTER_PORT",
+        "PL_GLOBAL_SEED",
+        "PL_SEED_WORKERS",
+        "WANDB_MODE",
+        "HOROVOD_FUSION_THRESHOLD",
+        "RANK",  # set by DeepSpeed
+    }
path: tests/conftest.py
text: @author:
```suggestion
        "RANK",  # set by DeepSpeed
        "POPLAR_ENGINE_OPTIONS",  # set by IPUPlugin
        # set by XLA
        "XRT_MESH_SERVICE_ADDRESS",
        "XRT_TORCH_DIST_ROOT",
    }
```
could also remove it on teardown.