Repository: Lightning-AI/pytorch-lightning
Pull request: Fix detection of next version in Fabric's CSVLogger
## What does this PR do?

Fix detection of next version in Fabric's CSVLogger. The issue was not properly handled in #17139 because our tests were incorrect in the first place.

Fixes #17886


cc @awaelchli @borda @Blaizzy @carmocca @justusschock

Problem statement:
Auto increment version for CSVLogger likeTensorBoardLogger in Fabric
### Description & Motivation

TensorBoardLogger auto increments next version, but CSVLogger doesn't, which I believe it's inconsistent.
```python
from lightning.fabric.loggers import TensorBoardLogger

logger = TensorBoardLogger("throw_away")
logger.log_metrics({"loss": 0.235, "acc": 0.75})
logger.finalize("success")
``` 
When running twice, it will create `version_0` then `version_1` folders.

However, 
```python
from lightning.fabric.loggers import CSVLogger

logger = CSVLogger("throw_away")
logger.log_metrics({"loss": 0.235, "acc": 0.75})
logger.finalize("success")
```

will give off the warning: /opt/conda/lib/python3.10/site-packages/lightning/fabric/loggers/csv_logs.py:186: UserWarning: Experiment logs directory throw_away/lightning_logs/version_0 exists and is not empty. Previous log files in this directory will be deleted when the new ones are saved!
  rank_zero_warn(


### Pitch

Given the following code, 
```python
from lightning.fabric.loggers import CSVLogger

logger = CSVLogger("throw_away")
logger.log_metrics({"loss": 0.235, "acc": 0.75})
logger.finalize("success")
```
When running this code twice (or again), the logger should create the next `version_{}` folder instead of warning.

### Alternatives

_No response_

### Additional context

_No response_

cc @borda @carmocca @justusschock @awaelchli

Hints from issue discussion:
@dinhanhx  your pitch is also giving the same warnings when they run consecutive time. 

After viewing the Implementation of csvlogger. I understood it's warning message that indicates previous version will be deleted and new version will be saved in that same directory.

@Borda  what do you think this feature has to be work on ? If yes can I work on this feature  ?

     
@dinhanhx 

<img width="866" alt="image" src="https://github.com/Lightning-AI/lightning/assets/88357044/1c4a49e9-eebc-4e8c-bafd-afedf2a406e6">

I have added this feature to my local repository and i will  give pull request.. If  maintainers accepts and merge soon you will get this feature  for you to use..
This should have been fixed by https://github.com/Lightning-AI/lightning/pull/17139 but apparently the fix was not correct. I'll take a look.

Commit message: stupid

Patch to review:
diff --git a/src/lightning/fabric/loggers/csv_logs.py b/src/lightning/fabric/loggers/csv_logs.py
index 312e48e6c2e76..7c26d139c3cf6 100644
--- a/src/lightning/fabric/loggers/csv_logs.py
+++ b/src/lightning/fabric/loggers/csv_logs.py
@@ -91,11 +91,6 @@ def version(self) -> Union[int, str]:
             self._version = self._get_next_version()
         return self._version
 
-    @property
-    def root_dir(self) -> str:
-        """Gets the save directory where the versioned CSV experiments are saved."""
-        return self._root_dir
-
     @property
     def log_dir(self) -> str:
         """The log directory for this run.
@@ -105,7 +100,7 @@ def log_dir(self) -> str:
         """
         # create a pseudo standard path
         version = self.version if isinstance(self.version, str) else f"version_{self.version}"
-        return os.path.join(self.root_dir, self.name, version)
+        return os.path.join(self._root_dir, self.name, version)
 
     @property
     @rank_zero_experiment
@@ -120,7 +115,7 @@ def experiment(self) -> "_ExperimentWriter":
         if self._experiment is not None:
             return self._experiment
 
-        os.makedirs(self.root_dir, exist_ok=True)
+        os.makedirs(self._root_dir, exist_ok=True)
         self._experiment = _ExperimentWriter(log_dir=self.log_dir)
         return self._experiment
 
@@ -153,14 +148,14 @@ def finalize(self, status: str) -> None:
         self.save()
 
     def _get_next_version(self) -> int:
-        root_dir = self.root_dir
+        versions_root = os.path.join(self._root_dir, self.name)
 
-        if not self._fs.isdir(root_dir):
-            log.warning("Missing logger folder: %s", root_dir)
+        if not self._fs.isdir(versions_root):
+            log.warning("Missing logger folder: %s", versions_root)
             return 0
 
         existing_versions = []
-        for d in self._fs.listdir(root_dir):
+        for d in self._fs.listdir(versions_root):
             full_path = d["name"]
             name = os.path.basename(full_path)
             if self._fs.isdir(full_path) and name.startswith("version_"):
diff --git a/tests/tests_fabric/loggers/test_csv.py b/tests/tests_fabric/loggers/test_csv.py
index 045a9aa67e848..0cbb11dd84c3e 100644
--- a/tests/tests_fabric/loggers/test_csv.py
+++ b/tests/tests_fabric/loggers/test_csv.py
@@ -21,23 +21,20 @@
 from lightning.fabric.loggers.csv_logs import _ExperimentWriter
 
 
-def test_file_logger_automatic_versioning(tmpdir):
+def test_file_logger_automatic_versioning(tmp_path):
     """Verify that automatic versioning works."""
-    root_dir = tmpdir.mkdir("exp")
-    root_dir.mkdir("version_0")
-    root_dir.mkdir("version_1")
-    logger = CSVLogger(root_dir=root_dir, name="exp")
+    os.makedirs(tmp_path / "exp" / "version_0")
+    os.makedirs(tmp_path / "exp" / "version_1")
+    logger = CSVLogger(root_dir=tmp_path, name="exp")
     assert logger.version == 2
 
 
-def test_file_logger_automatic_versioning_relative_root_dir(tmpdir, monkeypatch):
+def test_file_logger_automatic_versioning_relative_root_dir(tmp_path, monkeypatch):
     """Verify that automatic versioning works, when root_dir is given a relative path."""
-    root_dir = tmpdir.mkdir("exp")
-    logs_dir = root_dir.mkdir("logs")
-    logs_dir.mkdir("version_0")
-    logs_dir.mkdir("version_1")
-    monkeypatch.chdir(tmpdir)
-    logger = CSVLogger(root_dir="exp/logs", name="logs")
+    os.makedirs(tmp_path / "exp" / "logs" / "version_0")
+    os.makedirs(tmp_path / "exp" / "logs" / "version_1")
+    monkeypatch.chdir(tmp_path)
+    logger = CSVLogger(root_dir="exp", name="logs")
     assert logger.version == 2
 
 
@@ -71,7 +68,7 @@ def test_file_logger_no_name(tmpdir, name):
     logger = CSVLogger(root_dir=tmpdir, name=name)
     logger.log_metrics({"a": 1})
     logger.save()
-    assert os.path.normpath(logger.root_dir) == tmpdir  # use os.path.normpath to handle trailing /
+    assert os.path.normpath(logger._root_dir) == tmpdir  # use os.path.normpath to handle trailing /
     assert os.listdir(tmpdir / "version_0")
 
 


Code review comments:
diff_hunk: @@ -91,11 +91,6 @@ def version(self) -> Union[int, str]:
             self._version = self._get_next_version()
         return self._version
 
-    @property
-    def root_dir(self) -> str:
-        """Gets the save directory where the versioned CSV experiments are saved."""
-        return self._root_dir
path: src/lightning/fabric/loggers/csv_logs.py
text: @author:
I don't think I have another choice.

1. root_dir is ill-defined in the PL-CSVLogger
2. Reviewers requested in the original PR that the loggers in PL should inherit all code from the Fabric variants to reduce code duplication.
3. If the root_dir property is defined differently in the two implementations, but the shared code (through inheritance) assumes one definition, we have a problem. 



@user1:
Can you describe the difference in how PL and Fabric treat the `root_dir`?

And if this is irreconcilable, should `Fabric.root_dir` raise an exception to avoid silent errors (with a comment describing why)? Because the base interface does provide a default implementation which would not be right to call

@author:
In PL, `root_dir` is misleading since it doesn't point to the root, but rather to `os.path.join(self.save_dir, self.name)`. In Fabric, root_dir is what save_dir is in PL, giving its name an actual meaningful purpose.

> And if this is irreconcilable, should Fabric.root_dir raise an exception to avoid silent errors (with a comment describing why)?

You mean making `CSVLogger.root_dir`in Fabric raise an exception?

@user1:
> You mean making CSVLogger.root_dirin Fabric raise an exception?

Yes, sorry

@author:
Sorry, my bad. The removal is not necessary. Resolving

diff_hunk: @@ -21,23 +21,20 @@
 from lightning.fabric.loggers.csv_logs import _ExperimentWriter
 
 
-def test_file_logger_automatic_versioning(tmpdir):
+def test_file_logger_automatic_versioning(tmp_path):
     """Verify that automatic versioning works."""
-    root_dir = tmpdir.mkdir("exp")
-    root_dir.mkdir("version_0")
-    root_dir.mkdir("version_1")
-    logger = CSVLogger(root_dir=root_dir, name="exp")
+    os.makedirs(tmp_path / "exp" / "version_0")
path: tests/tests_fabric/loggers/test_csv.py
text: @user1:
Since you updated the test to use `tmp_path`, you can now use https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir