Repository: mlflow/mlflow
Pull request: Fix error due to names with absolute path returned by ftp.nlst
In vsftpd, `ftp.nlst` returns a list of file names with absolute path.
And, it will cause an error. To solve it, the path need to be removed
with basename.

Fixes https://github.com/mlflow/mlflow/issues/3197

## What changes are proposed in this pull request?

(Please fill in changes proposed in this fix)

## How is this patch tested?

(Details)

## Release Notes

### Is this a user-facing change?

- [ ] No. You can skip the rest of this section.
- [ ] Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

### What component(s), interfaces, languages, and integrations does this PR affect?
Components 
- [x] `area/artifacts`: Artifact stores and artifact logging
- [ ] `area/build`: Build and test infrastructure for MLflow
- [ ] `area/docs`: MLflow documentation pages
- [ ] `area/examples`: Example code
- [ ] `area/model-registry`: Model Registry service, APIs, and the fluent client calls for Model Registry
- [ ] `area/models`: MLmodel format, model serialization/deserialization, flavors
- [ ] `area/projects`: MLproject format, project running backends
- [ ] `area/scoring`: Local serving, model deployment tools, spark UDFs
- [ ] `area/server-infra`: MLflow server, JavaScript dev server
- [ ] `area/tracking`: Tracking Service, tracking client APIs, autologging

Interface 
- [ ] `area/uiux`: Front-end, user experience, JavaScript, plotting
- [ ] `area/docker`: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
- [ ] `area/sqlalchemy`: Use of SQLAlchemy in the Tracking Service or Model Registry
- [ ] `area/windows`: Windows support

Language 
- [ ] `language/r`: R APIs and clients
- [ ] `language/java`: Java APIs and clients
- [ ] `language/new`: Proposals for new client languages

Integrations
- [ ] `integrations/azure`: Azure and Azure ML integrations
- [ ] `integrations/sagemaker`: SageMaker integrations
- [ ] `integrations/databricks`: Databricks integrations

### How should the PR be classified in the release notes? Choose one:

- [ ] `rn/breaking-change` - The PR will be mentioned in the "Breaking Changes" section
- [ ] `rn/none` - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
- [ ] `rn/feature` - A new user-facing feature worth mentioning in the release notes
- [x] `rn/bug-fix` - A user-facing bug fix worth mentioning in the release notes
- [ ] `rn/documentation` - A user-facing documentation change worth mentioning in the release notes


Problem statement:
[BUG] Failed to load model from the FTP store
### Willingness to contribute
The MLflow Community encourages bug fix contributions. Would you or another member of your organization be willing to contribute a fix for this bug to the MLflow code base?

- [ ] Yes. I can contribute a fix for this bug independently.
- [x] Yes. I would be willing to contribute a fix for this bug with guidance from the MLflow community.
- [ ] No. I cannot contribute a bug fix at this time.

### System information

- **binary**
- **mlflow, version 1.9.1**
- **python 3.7.7**

### Describe the problem

I load a model from the FTP store:

```
torch_load_model('ftp://mlflow:mlflow@10.10.10.1/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model/model')
```

but, I get an error:

```
Traceback (most recent call last):
  File "/Users/shaneing/PycharmProjects/gswatch/examples/load_model_from_ftp.py", line 9, in <module>
    model = torch_load_model('ftp://mlflow:mlflow@10.10.10.1/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model/model')
            └ <function load_model at 0x12e375710>
  File "/usr/local/anaconda3/envs/gswatch3.7/lib/python3.7/site-packages/mlflow/pytorch/__init__.py", line 407, in load_model
    pytorch_conf = _get_flavor_configuration(model_path=local_model_path, flavor_name=FLAVOR_NAME)
                   │                                    │                             └ 'pytorch'
                   │                                    └ '/var/folders/px/ncc8244n0sj6zsh5s6k8v7g40000gn/T/tmpo81ye7y5/model'
                   └ <function _get_flavor_configuration at 0x12e36d7a0>
  File "/usr/local/anaconda3/envs/gswatch3.7/lib/python3.7/site-packages/mlflow/utils/model_utils.py", line 26, in _get_flavor_configuration
    RESOURCE_DOES_NOT_EXIST)
mlflow.exceptions.MlflowException: Could not find an "MLmodel" configuration file at "/var/folders/px/ncc8244n0sj6zsh5s6k8v7g40000gn/T/tmpo81ye7y5/model"
```

Because `ftp_artifact_repo._download_file` don't download file to temporary directory but `/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model/model` directory.

### Other info / logs

I can find the model from the FTP server:

```
root@f2159e0080b0:/home/vsftpd/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model/model# pwd
/home/vsftpd/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model/model
```

and find the downloaded model from local:

```
% pwd
/mlruns/8/6f11ca34312a44df88f6b86f2f224f66/artifacts/fake-model
```

### What component(s), interfaces, languages, and integrations does this bug affect?
Components 
- [x] `area/artifacts`: Artifact stores and artifact logging
- [ ] `area/build`: Build and test infrastructure for MLflow
- [ ] `area/docs`: MLflow documentation pages
- [ ] `area/examples`: Example code
- [ ] `area/model-registry`: Model Registry service, APIs, and the fluent client calls for Model Registry
- [ ] `area/models`: MLmodel format, model serialization/deserialization, flavors
- [ ] `area/projects`: MLproject format, project running backends
- [ ] `area/scoring`: Local serving, model deployment tools, spark UDFs
- [ ] `area/server-infra`: MLflow server, JavaScript dev server
- [ ] `area/tracking`: Tracking Service, tracking client APIs, autologging

Interface 
- [ ] `area/uiux`: Front-end, user experience, JavaScript, plotting
- [ ] `area/docker`: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
- [ ] `area/sqlalchemy`: Use of SQLAlchemy in the Tracking Service or Model Registry
- [ ] `area/windows`: Windows support

Language 
- [ ] `language/r`: R APIs and clients
- [ ] `language/java`: Java APIs and clients
- [ ] `language/new`: Proposals for new client languages

Integrations
- [ ] `integrations/azure`: Azure and Azure ML integrations
- [ ] `integrations/sagemaker`: SageMaker integrations
- [ ] `integrations/databricks`: Databricks integrations

Hints from issue discussion:
@shaneing Thanks for filing this. Does the model file exist in your FTP file store?
> @shaneing Thanks for filing this. Does the model file exist in your FTP file store?

Sorry, I have updated the issue. Please review it again.
@shaneing 

I wasn't able to reproduce the issue. Here's my code to log and load a pytorch model.

``````python
"""
# Command to setup an FTP server:

```
docker run -d --name ftpd_server -p 21:21 -p 30000-30009:30000-30009 -e "PUBLICHOST=localhost" \
    -e FTP_USER_NAME=mlflow -e FTP_USER_PASS=mlflow -e FTP_USER_HOME=/home/mlflow stilliard/pure-ftpd
```

Ref.: https://github.com/stilliard/docker-pure-ftpd
"""

import os
import random
import string
import torch

import mlflow
import mlflow.pytorch


def train_torch_model():
    # https://pytorch.org/tutorials/beginner/pytorch_with_examples.html#pytorch-nn
    N, D_in, H, D_out = 64, 1000, 100, 10
    x = torch.randn(N, D_in)
    y = torch.randn(N, D_out)
    model = torch.nn.Sequential(
        torch.nn.Linear(D_in, H),
        torch.nn.ReLU(),
        torch.nn.Linear(H, D_out),
    )
    loss_fn = torch.nn.MSELoss(reduction='sum')
    learning_rate = 1e-4
    for t in range(500):
        y_pred = model(x)
        loss = loss_fn(y_pred, y)
        model.zero_grad()
        loss.backward()
        with torch.no_grad():
            for param in model.parameters():
                param -= learning_rate * param.grad

    return model


def get_random_string(length):
    # https://pynative.com/python-generate-random-string/
    letters = string.ascii_lowercase
    return ''.join(random.choice(letters) for i in range(length))


expr_name = get_random_string(32)
artifact_location = "ftp://mlflow:mlflow@0.0.0.0:21/mlruns"

if mlflow.get_experiment_by_name(expr_name) is None:
    mlflow.create_experiment(expr_name, artifact_location)

mlflow.set_experiment(expr_name)


with mlflow.start_run():
    print('mlflow version:', mlflow.__version__)
    print('tracking URI:', mlflow.get_artifact_uri())
    print('artifact URI:', mlflow.get_tracking_uri())

    model = train_torch_model()
    mlflow.pytorch.log_model(model, 'model')
    uri = mlflow.get_artifact_uri()

    loaded_model = mlflow.pytorch.load_model(os.path.join(uri, "model", "model"))
    print(loaded_model)

``````

output:

```
mlflow version: 1.9.2.dev0
tracking URI: ftp://mlflow:mlflow@0.0.0.0:21/mlruns/7752bfdfb0df4c058e7f4fd422100562/artifacts
artifact URI: file:///Users/harutakakawamura/Desktop/repositories/mlflow/mlruns
Sequential(
  (0): Linear(in_features=1000, out_features=100, bias=True)
  (1): ReLU()
  (2): Linear(in_features=100, out_features=10, bias=True)
)
```
@shaneing Can you share your code to log a pytorch model?
@shaneing 
I was able to reproduce the same error by executing `mlflow.pytorch.load_model` against an empty directory. Could you check the model file exists in your FTP file store by running `ls /path/to/model_dir`?
> @shaneing
> I was able to reproduce the same error by executing `mlflow.pytorch.load_model` against an empty directory. Could you check the model file exists in your FTP file store by running `ls /path/to/model_dir`?

I have located the problem.  It will occur error when `ftp.nlst` return files with the absolute path. I am using image `bogem/ftp`.
@shaneing I was able to reproduce the same error with `bogem/ftp`.
I have the same problem. Changing the FTP Server resolved this particular issue. But when I register my model I can't used the downloaded registered model from the FTP sever.

I opened the above issue for this.
@harupy it's easy to fix this error by `list(map(lambda x: os.path.basename(x), artifact_files))`.
@shaneing I have confirmed inserting `artifact_files = list(map(lambda x: os.path.basename(x), artifact_files))` under the following line fixes the issue. 

https://github.com/mlflow/mlflow/blob/af4e753a0bc3fcc7ab2c74bf9c4ed459de6db1ef/mlflow/store/artifact/ftp_artifact_repo.py#L114

Would you be interested in making a PR?

Commit message: Clean up

Signed-off-by: shaneing <z.shane.ing@gmail.com>

Patch to review:
diff --git a/mlflow/store/artifact/ftp_artifact_repo.py b/mlflow/store/artifact/ftp_artifact_repo.py
index 7d96af5d7fe49..28cf09be13b1f 100644
--- a/mlflow/store/artifact/ftp_artifact_repo.py
+++ b/mlflow/store/artifact/ftp_artifact_repo.py
@@ -112,6 +112,8 @@ def list_artifacts(self, path=None):
                 return []
             artifact_files = ftp.nlst(list_dir)
             artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
+            # Make sure artifact_files is a list of file names because ftp.nlst may return absolute paths.
+            artifact_files = list(map(lambda x: os.path.basename(x), artifact_files))
             infos = []
             for file_name in artifact_files:
                 file_path = (file_name if path is None
diff --git a/tests/store/artifact/test_ftp_artifact_repo.py b/tests/store/artifact/test_ftp_artifact_repo.py
index 97fd414f34bf2..14861d5156256 100644
--- a/tests/store/artifact/test_ftp_artifact_repo.py
+++ b/tests/store/artifact/test_ftp_artifact_repo.py
@@ -66,6 +66,44 @@ def test_list_artifacts(ftp_mock):
     assert artifacts[1].file_size is None
 
 
+def test_list_artifacts_when_ftp_nlst_returns_absolute_paths(ftp_mock):
+    artifact_root_path = "/experiment_id/run_id/"
+    repo = FTPArtifactRepository("ftp://test_ftp"+artifact_root_path)
+
+    repo.get_ftp_client = MagicMock()
+    call_mock = MagicMock(return_value=ftp_mock)
+    repo.get_ftp_client.return_value = MagicMock(__enter__=call_mock)
+
+    # mocked file structure
+    #  |- file
+    #  |- model
+    #     |- model.pb
+
+    file_path = "file"
+    dir_path = "model"
+    file_size = 678
+    ftp_mock.cwd = MagicMock(side_effect=[None, ftplib.error_perm, None])
+    ftp_mock.nlst = MagicMock(return_value=[
+       posixpath.join(artifact_root_path, file_path),
+       posixpath.join(artifact_root_path, dir_path)
+    ])
+
+    ftp_mock.size = MagicMock(return_value=file_size)
+
+    artifacts = repo.list_artifacts(path=None)
+
+    ftp_mock.nlst.assert_called_once_with(artifact_root_path)
+    ftp_mock.size.assert_called_once_with(artifact_root_path + file_path)
+
+    assert len(artifacts) == 2
+    assert artifacts[0].path == file_path
+    assert artifacts[0].is_dir is False
+    assert artifacts[0].file_size == file_size
+    assert artifacts[1].path == dir_path
+    assert artifacts[1].is_dir is True
+    assert artifacts[1].file_size is None
+
+
 def test_list_artifacts_with_subdir(ftp_mock):
     artifact_root_path = "/experiment_id/run_id/"
     repo = FTPArtifactRepository("sftp://test_sftp"+artifact_root_path)


Code review comments:
diff_hunk: @@ -112,6 +112,8 @@ def list_artifacts(self, path=None):
                 return []
             artifact_files = ftp.nlst(list_dir)
             artifact_files = list(filter(lambda x: x != "." and x != "..", artifact_files))
+            # Make sure artifact_files is a list of file names because ftp.nlst may return absolute paths.
path: mlflow/store/artifact/ftp_artifact_repo.py
text: @user1:
@author Can you wrap this line to fix lint error?

@author:
Hang on a moment. I have installed `pylint`.