-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug(python): fix path handling in windows #724
Conversation
I also found this related bug: #673 (comment) might be worth it to deal with both in the same PR |
python/lancedb/util.py
Outdated
@@ -84,6 +85,18 @@ def fs_from_uri(uri: str) -> Tuple[pa_fs.FileSystem, str]: | |||
return pa_fs.FileSystem.from_uri(uri) | |||
|
|||
|
|||
def join_uri(base: Union[str, pathlib.Path], *parts: str) -> str: | |||
""" | |||
Join a URI with multiple parts, handling extra environment variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extra environment variables"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops that was generated by copilot and I forgot to change it >_<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test we can write for windows to validate this?
Oh we don't even have a Windows CI job... |
""" | ||
Join a URI with multiple parts, handling extra environment variables. | ||
""" | ||
if isinstance(base, pathlib.Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests?
8cdcf53
to
758f024
Compare
There seems to already be a test that fails on Windows due to path handling: https://github.com/lancedb/lancedb/actions/runs/7280056572/job/19837672999?pr=729 I have rebased your PR on my PR to add a Windows CI job. |
Use pathlib for local paths so that pathlib can handle the correct separator on windows. Closes #703
758f024
to
51c4bfa
Compare
a463488
to
86e2fab
Compare
86e2fab
to
a1fc4bd
Compare
good catch. I added a condition so that if the uri scheme is one letter then just return the uri instead of the netloc + path or path. Lmk if this addresses the cases you've encountered on windows. |
Use pathlib for local paths so that pathlib can handle the correct separator on windows. Closes lancedb#703 --------- Co-authored-by: Will Jones <willjones127@gmail.com>
Use pathlib for local paths so that pathlib can handle the correct separator on windows. Closes #703 --------- Co-authored-by: Will Jones <willjones127@gmail.com>
Use pathlib for local paths so that pathlib
can handle the correct separator on windows.
Closes #703