Repository: Lightning-AI/pytorch-lightning
Pull request: fix: MLFlowLogger artifact_path
## What does this PR do?

<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->

#20538
- It looks like the changes introduced in this PR broke mlflow logging of checkpoints
- `MlFlowClient.log_artifact` takes string for the `artifact_path` argument only
- I am getting the following error as a result. Not exactly helpful, but I figured the type difference was the reason.
  `mlflow.exceptions.MlflowException: Invalid artifact path: 'epoch=0-step=1'. Names may be treated as files in certain cases, and must not resolve to other names when treated as such. This name would resolve to 'epoch=0-step=1'`
- This change fixes it.

Fixes #20664

```python
# example.py

import os

import lightning as L
import torch
import torch.nn.functional as F
from torch import nn
from torch.utils.data import DataLoader
from torchvision import transforms
from torchvision.datasets import MNIST


class Encoder(nn.Module):
    def __init__(self):
        super().__init__()
        self.l1 = nn.Sequential(nn.Linear(28 * 28, 64), nn.ReLU(), nn.Linear(64, 3))

    def forward(self, x):
        return self.l1(x)


class Decoder(nn.Module):
    def __init__(self):
        super().__init__()
        self.l1 = nn.Sequential(nn.Linear(3, 64), nn.ReLU(), nn.Linear(64, 28 * 28))

    def forward(self, x):
        return self.l1(x)


class LitAutoEncoder(L.LightningModule):
    def __init__(self, encoder, decoder):
        super().__init__()
        self.encoder = encoder
        self.decoder = decoder

    def training_step(self, batch, batch_idx):
        # training_step defines the train loop.
        x, _ = batch
        x = x.view(x.size(0), -1)
        z = self.encoder(x)
        x_hat = self.decoder(z)
        loss = F.mse_loss(x_hat, x)
        return loss

    def configure_optimizers(self):
        optimizer = torch.optim.Adam(self.parameters(), lr=1e-3)
        return optimizer


dataset = MNIST(os.getcwd(), transform=transforms.ToTensor())
train_loader = DataLoader(dataset)
# model
autoencoder = LitAutoEncoder(Encoder(), Decoder())

# train model
mlflow_logger = L.pytorch.loggers.mlflow.MLFlowLogger(log_model=True)
trainer = L.Trainer(logger=mlflow_logger, max_steps=50)
trainer.fit(model=autoencoder, train_dataloaders=train_loader)
```

<!-- Does your PR introduce any breaking changes? If yes, please list them. -->

<details>
  <summary><b>Before submitting</b></summary>

- Was this **discussed/agreed** via a GitHub issue? (not for typos and docs)
- [ ] Did you read the [contributor guideline](https://github.com/Lightning-AI/lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [ ] Did you make sure your **PR does only one thing**, instead of bundling different changes together?
- Did you make sure to **update the documentation** with your changes? (if necessary)
- Did you write any **new necessary tests**? (not for typos and docs)
- [ ] Did you verify new and **existing tests pass** locally with your changes?
- Did you list all the **breaking changes** introduced by this pull request?
- Did you **update the CHANGELOG**? (not for typos, docs, test updates, or minor internal changes/refactors)

<!-- In the CHANGELOG, separate each item in the unreleased section by a blank line to reduce collisions -->

</details>

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the [review guidelines](https://github.com/Lightning-AI/lightning/wiki/Review-guidelines). In short, see the following bullet-list:

<details>
  <summary>Reviewer checklist</summary>

- [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
- [ ] Add labels and milestones (and optionally projects) to the PR so it can be classified

</details>

<!--

Did you have fun?

Make sure you had fun coding 🙃

-->


<!-- readthedocs-preview pytorch-lightning start -->
----
📚 Documentation preview 📚: https://pytorch-lightning--20669.org.readthedocs.build/en/20669/

<!-- readthedocs-preview pytorch-lightning end -->

Problem statement:
MLFlowLogger fails to log artifact on Windows
### Bug description

Error when training with "MLFlowLogger" and with `log_models="all"`, running on Windows:

```
mlflow.exceptions.MlflowException: Invalid artifact path: 'epoch=0-step=43654'. Names may be treated as files in certain cases, and must not resolve to other names when treated as such. This name would resolve to 'epoch=0-step=43654'.
```

I was able to find the reason for this error:

`MLFlowLogger` is calling `MLflowClient.log_artifact(...)` internally -- inside method `MLFlowLogger._scan_and_log_checkpoints(...)` -- passing the artifact path as a `pathlib.Path` object. However, MLFlow expects paths in the POSIX format, whereas `pathlib.Path` will use the current filesystem format.

Now, the actual reason for the error is due to an internal check that `MLFlow` does. It tries to verify that the path is already "normalized", that is, doesn't contain "." or "..". The way it does that is by calling `posixpath.normpath(...)` and checking if the output is the same as the original path. Because the original path is a "pathlib.Path" object, both paths do not match. I am assuming that if the platform was different from Windows, the comparison would match because the internal string representation of `pathlib.Path` would be the same.

### What version are you seeing the problem on?

v2.5

### How to reproduce the bug

```python

```

### Error messages and logs




### Environment

<details>
  <summary>Current environment</summary>

```
#- PyTorch Lightning Version (e.g., 2.5.0): 2.5.1
#- PyTorch Version (e.g., 2.5): 2.6.0
#- Python version (e.g., 3.12): 3.12.9
#- OS (e.g., Linux): Windows
#- CUDA/cuDNN version: 12.6
#- GPU models and configuration:
#- How you installed Lightning(`conda`, `pip`, source): pip
```

</details>


### More info

_No response_

Hints from issue discussion:

Commit message: fix: MLFlowLogger artifact_path

Patch to review:
diff --git a/src/lightning/pytorch/loggers/mlflow.py b/src/lightning/pytorch/loggers/mlflow.py
index 02396d8021633..4a89ec181d742 100644
--- a/src/lightning/pytorch/loggers/mlflow.py
+++ b/src/lightning/pytorch/loggers/mlflow.py
@@ -363,7 +363,7 @@ def _scan_and_log_checkpoints(self, checkpoint_callback: ModelCheckpoint) -> Non
             aliases = ["latest", "best"] if p == checkpoint_callback.best_model_path else ["latest"]
 
             # Artifact path on mlflow
-            artifact_path = Path(self._checkpoint_path_prefix) / Path(p).stem
+            artifact_path = str(Path(self._checkpoint_path_prefix, Path(p).stem))
 
             # Log the checkpoint
             self.experiment.log_artifact(self._run_id, p, artifact_path)


Code review comments:
diff_hunk: @@ -363,7 +363,7 @@ def _scan_and_log_checkpoints(self, checkpoint_callback: ModelCheckpoint) -> Non
             aliases = ["latest", "best"] if p == checkpoint_callback.best_model_path else ["latest"]
 
             # Artifact path on mlflow
-            artifact_path = Path(self._checkpoint_path_prefix) / Path(p).stem
+            artifact_path = str(Path(self._checkpoint_path_prefix, Path(p).stem))
path: src/lightning/pytorch/loggers/mlflow.py
text: @user1:
Instead of doing this string conversion, why not call the `as_posix()` function from the `Path` object. This should make this working on Windows as well. Please check this bug I filed: #20664 

`MLFlowClient` implementation expects paths in the POSIX format.

@author:
```suggestion
            artifact_path = Path(self._checkpoint_path_prefix, Path(p).stem).as_posix()
```

@author:
Based on the comment on the issue, does it mean that it is necessary to call `resolve()` on the `artifact_path`?

@user1:
I believe `resolve()` is for resolving symlinks and special path components. `artifact_path` is holds the path for the artifact in the `MLFlow` tracking system, not the real local filesystem path. I would argue that using `as_posix` is enough and there is no need to use `resolve()`.