Skip to content

Conversation

@littlebullGit
Copy link
Contributor

@littlebullGit littlebullGit commented Jul 19, 2025

Fix MLFlowLogger.save_dir Windows file URI handling

What does this PR do?

Fixes #20972

This PR fixes a bug in MLFlowLogger.save_dir where Windows absolute file URIs were being incorrectly parsed, resulting in malformed local paths that caused FileNotFoundError on Windows systems.

Problem:
When using [MLFlowLogger] with Windows absolute file URIs (e.g., file:///C:/Dev/example/mlruns), the [save_dir] property would return malformed paths like ///C:/Dev/example/mlruns instead of the expected C:/Dev/example/mlruns, causing file system operations to fail.

Solution:

  • Replace simple string slicing with proper URI parsing using urllib.parse.urlparse and urllib.request.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

Changes:

  1. Core Fix: Updated MLFlowLogger.save_dir property to use standard library URI parsing methods
  2. Test Coverage: Added comprehensive test [test_mlflow_logger_save_dir_file_uri_handling] covering:
    • Unix-style absolute file URIs
    • Windows-style absolute file URIs
    • Relative file URIs
    • Non-file URIs (should return None)
    • URIs with URL-encoded special characters

📚 Documentation preview 📚: https://pytorch-lightning--20988.org.readthedocs.build/en/20988/

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 19, 2025
@littlebullGit littlebullGit changed the title Fix MLFlowLogger.save_dir Windows file URI handling (#20972) Fix MLFlowLogger.save_dir Windows file URI handling (Fixes #20972) Jul 22, 2025
@littlebullGit
Copy link
Contributor Author

@lantiga @williamFalcon , @Borda @ethanwharris @tchaton @justusschock Let me know your comments. Any changes I need to get this PR approved? It has been 2 weeks.

@Borda Borda changed the title Fix MLFlowLogger.save_dir Windows file URI handling (Fixes #20972) Fix MLFlowLogger.save_dir Windows file URI handling Aug 11, 2025
@Borda Borda added logger Related to the Loggers logger: mlflow labels Aug 11, 2025
@littlebullGit littlebullGit force-pushed the fix/20972-mlflow-logger-windows-uri-from-master branch from 41d41c0 to c370a89 Compare August 12, 2025 23:40
@Borda
Copy link
Collaborator

Borda commented Aug 13, 2025

@littlebullGit this requires @williamFalcon only as he is the single code-owner for loggers

@littlebullGit littlebullGit force-pushed the fix/20972-mlflow-logger-windows-uri-from-master branch from 196e27e to d248b61 Compare August 13, 2025 22:21
@littlebullGit littlebullGit force-pushed the fix/20972-mlflow-logger-windows-uri-from-master branch from d0ef6b4 to 2025782 Compare August 14, 2025 01:22
@littlebullGit
Copy link
Contributor Author

The failed check has nothing to do with the code fix. We can rerun the test once @williamFalcon reviewed the code.

@Borda Borda marked this pull request as draft August 22, 2025 10:50
@littlebullGit littlebullGit force-pushed the fix/20972-mlflow-logger-windows-uri-from-master branch from 2025782 to 952e2ed Compare August 29, 2025 23:00
@littlebullGit littlebullGit marked this pull request as ready for review August 30, 2025 00:27
@littlebullGit
Copy link
Contributor Author

just rebased the PR

- 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 Lightning-AI#20972
- 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
@littlebullGit littlebullGit force-pushed the fix/20972-mlflow-logger-windows-uri-from-master branch from 6c52a05 to de52cd6 Compare September 2, 2025 02:32
@littlebullGit
Copy link
Contributor Author

build error did not seems related to my changes.

@Borda Borda marked this pull request as draft September 2, 2025 13:15
@littlebullGit littlebullGit marked this pull request as ready for review December 6, 2025 01:25
@codecov
Copy link

codecov bot commented Dec 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79%. Comparing base (79ffe50) to head (82b7cbc).
✅ All tests successful. No failed tests found.

❗ There is a different number of reports uploaded between BASE (79ffe50) and HEAD (82b7cbc). Click for more details.

HEAD has 2815 uploads less than BASE
Flag BASE (79ffe50) HEAD (82b7cbc)
cpu 659 30
lightning_fabric 165 0
pytest 330 0
python3.12 197 9
python3.12.7 198 9
lightning 330 15
python3.11 132 6
python3.10 66 3
python 66 3
pytorch2.1 66 6
pytest-full 329 30
pytorch_lightning 164 15
pytorch2.6 33 3
pytorch2.4.1 33 3
pytorch2.3 33 3
pytorch2.2.2 33 3
pytorch2.5.1 33 3
pytorch2.9 33 3
pytorch2.7 33 3
pytorch2.8 32 3
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #20988     +/-   ##
=========================================
- Coverage      87%      79%     -8%     
=========================================
  Files         269      266      -3     
  Lines       23804    23755     -49     
=========================================
- Hits        20626    18732   -1894     
- Misses       3178     5023   +1845     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logger: mlflow logger Related to the Loggers pl Generic label for PyTorch Lightning package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MLFlowLogger.save_dir mishandles absolute file: URIs on Windows

4 participants