BUG: support PathLike paths in DataSource methods#31906
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
ikrommyd
left a comment
There was a problem hiding this comment.
I'd suggest opening an issue regarding what things don't work because indeed there was pathlib.Path added in the docstrings in #23822. Then one can link the PR as closing that issue.
It looks like some cases were probably missed during the testing of #23822. if you say that things do not work while they claim to support pathlib.Path.
Since the change generalizes support from pathlib.Path to path-like objects, could you update the remaining str or pathlib.Path docstrings in _datasource.py to str or path-like for consistency?
|
@ikrommyd OK...I will do it |
|
Thanks @ikrommyd. I opened #31909 to describe the documented I also updated the remaining Local validation for this update:
|
|
I'll do a pass over this as soon as I can. In the meantime, please fix the linting failure in the tests (you will get locally with |
|
Thanks @ikrommyd! I fixed the lint failure in Local validation now passes: The remaining CircleCI checks still look pending on my side and may need maintainer approval/triggering for this fork PR. Could you please rerun/trigger CI when you have a chance? |
ikrommyd
left a comment
There was a problem hiding this comment.
I think it's generally alright and a reasonable improvement. I'm not sure if I would call this a bug fix or an enhancement though tbh. I'm leaning towards enhancement as it adds path-like support even though the docstrings used to incorrectly claim pathlib.Path was supported. Any suggestions from anyone?
I'd want someone else to look at the typing stub changes as I'm not good at all with typing cc @jorenham
| mode : str, optional | ||
| Mode to open `path`. Mode 'r' for reading, 'w' for writing, 'a' to | ||
| append. Available modes depend on the type of object specified by | ||
| path. Default is 'r'. | ||
| destpath : str, optional | ||
| Path to the directory where the source file gets downloaded to for | ||
| use. If `destpath` is None, a temporary directory will be created. | ||
| The default path is the current directory. |
There was a problem hiding this comment.
I think these also should be be updated to include path-like now no?
There was a problem hiding this comment.
Good catch, thanks. I’ll update that.
jorenham
left a comment
There was a problem hiding this comment.
The typing changes are good 👌
ikrommyd
left a comment
There was a problem hiding this comment.
I think this is good. if the maintainers agree, I'd probably call this PR an enhancement and not a bug.
|
Thanks @VectorPeak |
### PR summary
Closes #31909.
This PR makes
numpy.lib._datasourceacceptpathlib.Pathand other string path-like local paths in the same places that already accepted string paths. The affected datasource entry points now normalize path inputs withos.fspath()before the existing URL, cache, absolute-path, and file-opening logic runs.Previously, direct
pathlib.Pathinputs could reach string-specific operations such aspath.split(...)without conversion. This affectedDataSource.abspath,DataSource.exists,DataSource.open, the module-levelnumpy.lib._datasource.open, andRepositorypath resolution. The change is intentionally scoped to datasource path normalization; URL handling, cache lookup order, compressed-file opener selection, write-mode URL rejection, and sandboxing logic are left unchanged.The runtime tests cover
Pathinputs forDataSource.open,DataSource.exists,DataSource.abspath,Repository.abspath,Repository.exists,Repository.open, and the module-levelopenhelper. The typing fixtures now use_typeshed.StrPathsopathlib.Pathis accepted while bytes-path cases remain rejected.Validation:
Pathcases and preserved URL-string detection (failure_count 0).python -m py_compile numpy/lib/_datasource.pypassed with Python 3.12.13.git diff --checkpassed.spin build --cleanwas attempted locally, but this Windows environment hit MSVCfatal error C1001internal compiler errors while compiling unrelated linalg sources, so I could not run the focused pytest target from a completed local build.First time committer introduction
I am contributing a small bug fix to align
DataSourcebehavior with its documented path-like support.AI Disclosure
AI tools were used to help identify the failing path-like case, draft and validate the patch, and prepare this PR description. The proposed code changes and PR text were reviewed before submission.