Open kurtmckee opened 4 days ago
hey @kurtmckee I can work on this issue ...can you assign me.
@kanishka-coder0809 As I wrote in the text above, I'd like to fix this myself but I'm waiting for confirmation how to proceed, because I think this issue implicates importlib_resources
and the standard library.
In any case, I don't have GitHub permissions to assign this to anyone.
I'm encountering this issue while trying to add 3.13 support to PyTorch.
cc @brettcannon @jaraco @warsaw @FFY00
Thanks Kurt for the detailed report elucidating the issue. I do think I agree that adding the required parameter to the default read_text
implementation sounds right.
When reasoning about where to create the fix, I'm fine if it goes here or in importlib_resources
. I personally find it easier and faster to develop on importlib_resources
first, in part because it will apply to older Python versions (3.9+) and also because the test suite is easier to run (just run tox
). But if you choose to develop it here on CPython first, that's fine and I'll backport it to importlib_resources
(and thus also get immediate feedback when it's released in short order).
At first, I was thinking that this change would be backward-incompatible, because it's a change to a protocol (on which downstream consumers rely), but since it's a change to a concrete method on the protocol, I'm thinking a backward-compatible change is possible, but we'll want to think carefully about that aspect of the change.
Please feel free to proceed with a proposed fix.
Bug report
Bug description:
I'm writing a custom importer and discovered that the function signature for
importlib.abc.Traversable.read_text()
is incompatible with the usage inimportlib.resources._functional.read_text()
, specifically on Python 3.13.importlib.abc.Traversable.read_text()
is a concrete method; its implementation calls the.open()
method, which is an abstract method that must be implemented. The expectation is therefore that implementing.open()
in a Traversable subclass is sufficient for.read_text()
to work.Note below that the
.read_text()
method is not marked as abstract, and includes only one parameter:encoding
:https://github.com/python/cpython/blob/30aeb00d367d0cc9e5a7603371636cddea09f1c0/Lib/importlib/resources/abc.py#L84-L90
Application code that attempts to read a package resource, like
importlib.resources.read_text(module, "resource.txt")
ultimately leads to a call toimportlib.resources._functional.read_text()
, which attempts to call the.read_text()
method of a Traversable subclass, but includes anerrors
parameter that doesn't exist in Traversable's default concrete method:https://github.com/python/cpython/blob/30aeb00d367d0cc9e5a7603371636cddea09f1c0/Lib/importlib/resources/_functional.py#L28-L32
Consequently, it appears to be necessary for all Traversable subclasses to not only re-implement its concrete
.read_text()
method, but also to override its signature.I think that the Traversable
.read_text()
method signature, and the call site inimportlib.resources._functional.read_text()
, need to align with each other.I'd like to submit a PR for this! However, I would like confirmation that an
errors
parameter should be added to theTraversable.read_text()
method.Note that adding an
errors
parameter was previously discussed in #88368.Demonstration of TypeError bug
CPython versions tested on:
3.13
Operating systems tested on:
Linux