From 972f512e0aae03957facf4fcba430507fb064b75 Mon Sep 17 00:00:00 2001 From: LittlebullGit Date: Sat, 19 Jul 2025 11:16:27 -0400 Subject: [PATCH 1/2] Fix MLFlowLogger.save_dir Windows file URI handling (#20972) - Replace simple string slicing with urllib.parse.urlparse and url2pathname - Properly handle Windows absolute file URIs (e.g., file:///C:/path) - Add comprehensive tests for various file URI formats - Fix malformed paths like ///C:/path becoming C:/path on Windows Fixes #20972 --- src/lightning/pytorch/loggers/mlflow.py | 6 +++- tests/tests_pytorch/loggers/test_mlflow.py | 32 ++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/lightning/pytorch/loggers/mlflow.py b/src/lightning/pytorch/loggers/mlflow.py index ff9b2b0d7e542..1d841febf7f55 100644 --- a/src/lightning/pytorch/loggers/mlflow.py +++ b/src/lightning/pytorch/loggers/mlflow.py @@ -300,7 +300,11 @@ def save_dir(self) -> Optional[str]: """ if self._tracking_uri.startswith(LOCAL_FILE_URI_PREFIX): - return self._tracking_uri[len(LOCAL_FILE_URI_PREFIX) :] + from urllib.parse import urlparse + from urllib.request import url2pathname + + parsed_uri = urlparse(self._tracking_uri) + return url2pathname(parsed_uri.path) return None @property diff --git a/tests/tests_pytorch/loggers/test_mlflow.py b/tests/tests_pytorch/loggers/test_mlflow.py index c7f9dbe1fe2c6..95125d9c6415c 100644 --- a/tests/tests_pytorch/loggers/test_mlflow.py +++ b/tests/tests_pytorch/loggers/test_mlflow.py @@ -427,3 +427,35 @@ def test_set_tracking_uri(mlflow_mock): mlflow_mock.set_tracking_uri.assert_not_called() _ = logger.experiment mlflow_mock.set_tracking_uri.assert_called_with("the_tracking_uri") + + +@mock.patch("lightning.pytorch.loggers.mlflow._get_resolve_tags", Mock()) +def test_mlflow_logger_save_dir_file_uri_handling(mlflow_mock): + """Test that save_dir correctly handles file URIs, especially on Windows.""" + # Test Unix-style absolute file URI + logger = MLFlowLogger(tracking_uri="file:///home/user/mlruns") + expected_unix = "/home/user/mlruns" + assert logger.save_dir == expected_unix + + # Test Windows-style absolute file URI + logger_win = MLFlowLogger(tracking_uri="file:///C:/Dev/example/mlruns") + # On Windows, url2pathname converts file:///C:/path to C:\path + # On Unix, it converts to /C:/path, but we test the actual behavior + import platform + + expected_win = "C:\\Dev\\example\\mlruns" if platform.system() == "Windows" else "/C:/Dev/example/mlruns" + assert logger_win.save_dir == expected_win + + # Test relative file URI + logger_rel = MLFlowLogger(tracking_uri="file:./mlruns") + expected_rel = "./mlruns" + assert logger_rel.save_dir == expected_rel + + # Test non-file URI (should return None) + logger_http = MLFlowLogger(tracking_uri="http://localhost:8080") + assert logger_http.save_dir is None + + # Test file URI with special characters and spaces + logger_special = MLFlowLogger(tracking_uri="file:///path/with%20spaces/mlruns") + expected_special = "/path/with spaces/mlruns" + assert logger_special.save_dir == expected_special From 41d41c0cbaa5b9353b284286366635856c9c70f8 Mon Sep 17 00:00:00 2001 From: LittlebullGit Date: Sat, 19 Jul 2025 12:01:15 -0400 Subject: [PATCH 2/2] Fix compatibility with legacy file URI format - Handle both proper file URIs (file:///path) and legacy format (file:/path) - Proper URIs use urlparse/url2pathname for Windows compatibility - Legacy format used by constructor returns path as-is - Update tests to cover both formats and Windows behavior --- src/lightning/pytorch/loggers/mlflow.py | 17 ++++++--- tests/tests_pytorch/loggers/test_mlflow.py | 42 +++++++++++++--------- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/lightning/pytorch/loggers/mlflow.py b/src/lightning/pytorch/loggers/mlflow.py index 1d841febf7f55..a904a79861e3a 100644 --- a/src/lightning/pytorch/loggers/mlflow.py +++ b/src/lightning/pytorch/loggers/mlflow.py @@ -300,11 +300,18 @@ def save_dir(self) -> Optional[str]: """ if self._tracking_uri.startswith(LOCAL_FILE_URI_PREFIX): - from urllib.parse import urlparse - from urllib.request import url2pathname - - parsed_uri = urlparse(self._tracking_uri) - return url2pathname(parsed_uri.path) + # Handle both proper file URIs (file:///path) and legacy format (file:/path) + uri_without_prefix = self._tracking_uri[len(LOCAL_FILE_URI_PREFIX) :] + + # If it starts with ///, it's a proper file URI, use urlparse + if uri_without_prefix.startswith("///"): + from urllib.parse import urlparse + from urllib.request import url2pathname + + parsed_uri = urlparse(self._tracking_uri) + return url2pathname(parsed_uri.path) + # Legacy format: file:/path or file:./path - return as-is + return uri_without_prefix return None @property diff --git a/tests/tests_pytorch/loggers/test_mlflow.py b/tests/tests_pytorch/loggers/test_mlflow.py index 95125d9c6415c..9de0ad23a93cf 100644 --- a/tests/tests_pytorch/loggers/test_mlflow.py +++ b/tests/tests_pytorch/loggers/test_mlflow.py @@ -432,30 +432,38 @@ def test_set_tracking_uri(mlflow_mock): @mock.patch("lightning.pytorch.loggers.mlflow._get_resolve_tags", Mock()) def test_mlflow_logger_save_dir_file_uri_handling(mlflow_mock): """Test that save_dir correctly handles file URIs, especially on Windows.""" - # Test Unix-style absolute file URI - logger = MLFlowLogger(tracking_uri="file:///home/user/mlruns") - expected_unix = "/home/user/mlruns" - assert logger.save_dir == expected_unix - - # Test Windows-style absolute file URI - logger_win = MLFlowLogger(tracking_uri="file:///C:/Dev/example/mlruns") - # On Windows, url2pathname converts file:///C:/path to C:\path - # On Unix, it converts to /C:/path, but we test the actual behavior import platform + # Test proper Windows-style absolute file URI (the main fix) + logger_win = MLFlowLogger(tracking_uri="file:///C:/Dev/example/mlruns") + result_win = logger_win.save_dir expected_win = "C:\\Dev\\example\\mlruns" if platform.system() == "Windows" else "/C:/Dev/example/mlruns" - assert logger_win.save_dir == expected_win + assert result_win == expected_win - # Test relative file URI + # Test proper Unix-style absolute file URI + logger_unix = MLFlowLogger(tracking_uri="file:///home/user/mlruns") + result_unix = logger_unix.save_dir + expected_unix = "\\home\\user\\mlruns" if platform.system() == "Windows" else "/home/user/mlruns" + assert result_unix == expected_unix + + # Test proper file URI with special characters and spaces + logger_special = MLFlowLogger(tracking_uri="file:///path/with%20spaces/mlruns") + result_special = logger_special.save_dir + expected_special = "\\path\\with spaces\\mlruns" if platform.system() == "Windows" else "/path/with spaces/mlruns" + assert result_special == expected_special + + # Test legacy format used by constructor (file:/path - should return as-is) + logger_legacy = MLFlowLogger(tracking_uri="file:/tmp/mlruns") + result_legacy = logger_legacy.save_dir + expected_legacy = "/tmp/mlruns" + assert result_legacy == expected_legacy + + # Test legacy relative format logger_rel = MLFlowLogger(tracking_uri="file:./mlruns") + result_rel = logger_rel.save_dir expected_rel = "./mlruns" - assert logger_rel.save_dir == expected_rel + assert result_rel == expected_rel # Test non-file URI (should return None) logger_http = MLFlowLogger(tracking_uri="http://localhost:8080") assert logger_http.save_dir is None - - # Test file URI with special characters and spaces - logger_special = MLFlowLogger(tracking_uri="file:///path/with%20spaces/mlruns") - expected_special = "/path/with spaces/mlruns" - assert logger_special.save_dir == expected_special