Skip to content

fix: remove erroneous lstrip("/") in FileLock path construction#2187

Open
yangrq1018 wants to merge 1 commit intomicrosoft:mainfrom
yangrq1018:fix/filelock-path
Open

fix: remove erroneous lstrip("/") in FileLock path construction#2187
yangrq1018 wants to merge 1 commit intomicrosoft:mainfrom
yangrq1018:fix/filelock-path

Conversation

@yangrq1018
Copy link
Copy Markdown

Summary

lstrip("/") on pr.path in MLflowExpManager._get_or_create_exp turns absolute file:// URIs into relative paths, causing mlruns/filelock to be created under a spurious home/<user>/<cwd>/... directory tree relative to the working directory.

  • Root cause: In qlib/workflow/expm.py (line 236), urlparse on a local file:///... URI produces netloc="" and path="/home/.../mlruns". The .lstrip("/") strips the leading /, so os.path.join("", "home/.../mlruns", "filelock") yields a relative path instead of the correct absolute path.
  • Introduced in: a0cef03 ("update python version update python version #1868") as part of a bulk lint/CI cleanup — not an intentional logic change.
  • Original behavior: commit 45ea4ba ("Add file lock for MLflowExpManager Add file lock for MLflowExpManager #619") correctly used pr.path without stripping.

Fix

Remove the .lstrip("/") call. os.path.join("", "/absolute/path", "filelock") already produces the correct absolute path when netloc is empty.

- with FileLock(Path(os.path.join(pr.netloc, pr.path.lstrip("/"), "filelock"))):
+ with FileLock(Path(os.path.join(pr.netloc, pr.path, "filelock"))):

Related note

A similar misuse of lstrip exists in qlib/workflow/recorder.py (lines 321–323), where .lstrip("file:") is used to strip a URI prefix. Since str.lstrip strips characters (not a prefix), this can over-strip in edge cases. The correct approach would be str.removeprefix("file:") (Python 3.9+). This is not addressed in this PR.

🤖 Generated with Claude Code

The lstrip("/") on pr.path was introduced in a0cef03 as part of a bulk
lint/CI cleanup and inadvertently turned absolute file:// URIs into
relative paths, causing mlruns/filelock to be created under a spurious
home/<user>/... directory tree relative to cwd.

Restoring the original behavior where os.path.join with an empty netloc
and an absolute pr.path naturally produces the correct absolute path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant