Closed befeleme closed 5 months ago
Thanks! I know pathlib
has been rearchitectured considerably in 3.13 to allow for user subclassing support, which would explain trio.Path
breaking. We may want/need to rethink how it functions, to take advantage of those changes better.
Oh I guess even the test_timeouts_raise_value_error
error stems from trio.Path
, so looks like that's the "only" thing that needs addressing.
I think this is easily fixed with: https://github.com/etianen/trio/tree/dh/python-3.13
I've no idea how to test this against an unreleased Python, however.
I think this is easily fixed with: https://github.com/etianen/trio/tree/dh/python-3.13
I've no idea how to test this against an unreleased Python, however.
GitHub actions does have "3.13.0-alpha.3"
, so you can open a PR and add it to the ci.yml
file.
Seems to just be a cryptography
error now:
https://github.com/python-trio/trio/pull/2955
Would the thing to do now be to remove the failing test matrix item and mark my PR as ready for review?
The fix in #2955 has been merged. Can this issue be closed now, given the remaining problems are with a 3rd-party lib?
I took the fix and built trio again for Fedora, the same 8 tests failed. See logs: https://download.copr.fedorainfracloud.org/results/@python/python3.13/fedora-rawhide-x86_64/07022101-python-trio/builder-live.log.gz I'll be happy to test any coming fix in our build system where we build with Python 3.13.0 alphas repeatedly.
Tbh I think we should wait for 3.13 betas -> cffi release -> cryptography, for both the faster feedback cycle from CI and more guarantee that nothing will change
I'll take a look at those logs later. I'm confused about why the GHA CI only had cryptography errors.
How thoroughly embarrassing... the crypto errors were masking the Path
errors.
I'd like to have another go at this... I've got Python 3.13 installed locally now so can test the Path parts in isolation of the broken crypto
parts.
I think I know what the issue is. The current Path
relies on a AsyncAutoWrapperType
metaclass that introspects a "forwards" and a "wrapped" type, combining these into a new type. The problem seems to be that it relies on the __dict__
of the "forwards" and "wrapped" types, which has broken with recent changes to the pathlib
class hierarchy.
In Python 3.12:
>>> Path.__dict__["is_dir"]
<function Path.is_dir at 0x103415da0>
In Python 3.13:
>>> Path.__dict__["is_dir"]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
Path.__dict__["is_dir"]
~~~~~~~~~~~~~^^^^^^^^^^
I think this approach made too many assumptions about class hierarchy, and will likely break again even if fixed. I'd like to propose a dumber approach that will always work:
Let's lose the metaclass and automatic wrapping, and manually define and wrap the methods in trio.Path
. This sounds like it would be boilerplate nonsense, but I suspect it would actually be less code, because:
Path
wrapping.This new approach would be less brittle, more explicit and run (slightly) faster. It would probably be a simpler implementation.
I get your point about waiting until Python 3.13 beta... but I think this is a good change in of itself, and it should be resilient to any other pathlib
changes. I'd like to give it a go, but only so long as it's considered a good idea.
I do think that might be a better approach, though we would want to have lots of helper methods, and for most perhaps define them like rename = convert(PurePath.rename)
. Since 3.13 has added the ability for users to subclass pathlib classes (for representing things like say paths inside archives or on network services), we should probably also consider if we could handle those too.
Yes, I was anticipating having a small handful of wrapper helpers to make this as un-boiler-platy as possible. The things to handle are likely:
I wonder how that would work with path subclasses though? Like, in this case we're explicitly wrapping pathlib.Path
to make a whole new path-like class. We could make a factory method that takes a Path
and spits out a wrapped Path
delegating to that subclass, but that starts to fall into the rabbit-hole of automatically detecting the type of wrapping required for any new methods defined on that Path
subclass.
I'm tempted to say that we ignore path subclasses and get this working with the simpler implementation. After all, Path subclasses are a bit niche. Alternatively, we'd define trio.Path
as something like:
class Path:
wrapped_cls: ClassVar[type[Path]] = Path
wrapped: Path
This would allow a subclass of trio.Path
to be created delegating to a different wrapped path, with additional methods added as needed. ~I think this is maybe extra complexity and runtime overhead that isn't needed.~ Nah, it's cool
Well trio.Path
just contains a pathlib.Path
object, all we’d need to do is ensure it also accepts those subclasses.
Yeah, I changed my mind almost immediately after saying it would be a hassle.
So I'm happy to work on this, providing people are cool with it being a thing. Let me know.
Current implementation is mostly @TeamSpen210's brainchild, so I think you can go ahead~
This is ready for review, I think. I'm pretty happy with the new implementation, which is smaller and adds a nice new extra capability. #2959
But there's a problem with pyright --verifytypes
that I'd like some advice one.
Closing as the PR got merged and tests seem to pass on 3.13.0a6 (at least on my machine).
For some reason just adding 3.13 to CI fails -- I tried yesterday. But yeah that's tracked in a separate issue (the general CI improvements one)
I used the fix from https://github.com/python-trio/trio/pull/2918 to attempt to build trio in Fedora with python 3.13.0a3. This is an ongoing effort to integrate new Python as soon as possible and provide feedback to all interested parties. The result of the attempted build is that 8 tests fail: