Skip to content
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

[Heads up] Test failures with Python 3.13.0a3 #2944

Closed
befeleme opened this issue Feb 2, 2024 · 20 comments
Closed

[Heads up] Test failures with Python 3.13.0a3 #2944

befeleme opened this issue Feb 2, 2024 · 20 comments

Comments

@befeleme
Copy link

befeleme commented Feb 2, 2024

I used the fix from #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:

=================================== FAILURES ===================================
___________________ test_compare_async_stat_methods[is_dir] ____________________

method_name = 'is_dir'

    @pytest.mark.parametrize("method_name", ["is_dir", "is_file"])
    async def test_compare_async_stat_methods(method_name: str) -> None:
>       method, async_method = method_pair(".", method_name)

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:119: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:26: in method_pair
    return getattr(sync_path, method_name), getattr(async_path, method_name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = trio.Path('.'), name = 'is_dir'

    def __getattr__(self, name):
        if name in self._forward:
            value = getattr(self._wrapped, name)
            return rewrap_path(value)
>       raise AttributeError(name)
E       AttributeError: is_dir

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_path.py:236: AttributeError
___________________ test_compare_async_stat_methods[is_file] ___________________

method_name = 'is_file'

    @pytest.mark.parametrize("method_name", ["is_dir", "is_file"])
    async def test_compare_async_stat_methods(method_name: str) -> None:
>       method, async_method = method_pair(".", method_name)

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:119: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:26: in method_pair
    return getattr(sync_path, method_name), getattr(async_path, method_name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = trio.Path('.'), name = 'is_file'

    def __getattr__(self, name):
        if name in self._forward:
            value = getattr(self._wrapped, name)
            return rewrap_path(value)
>       raise AttributeError(name)
E       AttributeError: is_file

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_path.py:236: AttributeError
_________________________ test_forward_methods_rewrap __________________________

path = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_forward_methods_rewrap0/test')
tmp_path = PosixPath('/tmp/pytest-of-mockbuild/pytest-0/test_forward_methods_rewrap0')

    async def test_forward_methods_rewrap(path: trio.Path, tmp_path: pathlib.Path) -> None:
        with_name = path.with_name("foo")
>       with_suffix = path.with_suffix(".py")

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:145: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_forward_methods_rewrap0/test')
name = 'with_suffix'

    def __getattr__(self, name):
        if name in self._forward:
            value = getattr(self._wrapped, name)
            return rewrap_path(value)
>       raise AttributeError(name)
E       AttributeError: with_suffix

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_path.py:236: AttributeError
_____________________ test_forward_methods_without_rewrap ______________________

path = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_forward_methods_without_r0')

    async def test_forward_methods_without_rewrap(path: trio.Path) -> None:
        path = await path.parent.resolve()
    
>       assert path.as_uri().startswith("file:///")
E       AttributeError: 'coroutine' object has no attribute 'startswith'

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:160: AttributeError
_______________________________ test_globmethods _______________________________

path = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_globmethods0/test')

    async def test_globmethods(path: trio.Path) -> None:
        # Populate a directory tree
        await path.mkdir()
        await (path / "foo").mkdir()
>       await (path / "foo" / "_bar.txt").write_bytes(b"")

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:231: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_globmethods0/test/foo/_bar.txt')
name = 'write_bytes'

    def __getattr__(self, name):
        if name in self._forward:
            value = getattr(self._wrapped, name)
            return rewrap_path(value)
>       raise AttributeError(name)
E       AttributeError: write_bytes. Did you mean: 'write_text'?

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_path.py:236: AttributeError
_________________________________ test_iterdir _________________________________

path = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_iterdir0/test')

    async def test_iterdir(path: trio.Path) -> None:
        # Populate a directory
        await path.mkdir()
        await (path / "foo").mkdir()
>       await (path / "bar.txt").write_bytes(b"")

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:260: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = trio.Path('/tmp/pytest-of-mockbuild/pytest-0/test_iterdir0/test/bar.txt')
name = 'write_bytes'

    def __getattr__(self, name):
        if name in self._forward:
            value = getattr(self._wrapped, name)
            return rewrap_path(value)
>       raise AttributeError(name)
E       AttributeError: write_bytes. Did you mean: 'write_text'?

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_path.py:236: AttributeError
______________________________ test_classmethods _______________________________

    async def test_classmethods() -> None:
>       assert isinstance(await trio.Path.home(), trio.Path)
E       AttributeError: type object 'Path' has no attribute 'home'

../../BUILDROOT/python-trio-0.23.1-4.fc40.x86_64/usr/lib/python3.13/site-packages/trio/_tests/test_path.py:271: AttributeError
_______________________ test_timeouts_raise_value_error ________________________

cls = <class '_pytest.runner.CallInfo'>
func = <function call_runtest_hook.<locals>.<lambda> at 0x7f3b396119e0>
when = 'call'
reraise = (<class '_pytest.outcomes.Exit'>, <class 'KeyboardInterrupt'>)

    @classmethod
    def from_call(
        cls,
        func: "Callable[[], TResult]",
        when: "Literal['collect', 'setup', 'call', 'teardown']",
        reraise: Optional[
            Union[Type[BaseException], Tuple[Type[BaseException], ...]]
        ] = None,
    ) -> "CallInfo[TResult]":
        """Call func, wrapping the result in a CallInfo.
    
        :param func:
            The function to call. Called without arguments.
        :param when:
            The phase in which the function is called.
        :param reraise:
            Exception or exceptions that shall propagate if raised by the
            function, instead of being wrapped in the CallInfo.
        """
        excinfo = None
        start = timing.time()
        precise_start = timing.perf_counter()
        try:
>           result: Optional[TResult] = func()

/usr/lib/python3.13/site-packages/_pytest/runner.py:341: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.13/site-packages/_pytest/runner.py:262: in <lambda>
    lambda: ihook(item=item, **kwds), when=when, reraise=reraise
/usr/lib/python3.13/site-packages/pluggy/_hooks.py:493: in __call__
    return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
/usr/lib/python3.13/site-packages/pluggy/_manager.py:115: in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
/usr/lib/python3.13/site-packages/_pytest/unraisableexception.py:88: in pytest_runtest_call
    yield from unraisable_exception_runtest_hook()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def unraisable_exception_runtest_hook() -> Generator[None, None, None]:
        with catch_unraisable_exception() as cm:
            yield
            if cm.unraisable:
                if cm.unraisable.err_msg is not None:
                    err_msg = cm.unraisable.err_msg
                else:
                    err_msg = "Exception ignored in"
                msg = f"{err_msg}: {cm.unraisable.object!r}\n\n"
                msg += "".join(
                    traceback.format_exception(
                        cm.unraisable.exc_type,
                        cm.unraisable.exc_value,
                        cm.unraisable.exc_traceback,
                    )
                )
>               warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
E               pytest.PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object Path.as_uri at 0x7f3b393ba140>
E               
E               Traceback (most recent call last):
E                 File "/usr/lib64/python3.13/warnings.py", line 688, in _warn_unawaited_coroutine
E                   warn(msg, category=RuntimeWarning, stacklevel=2, source=coro)
E                   ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E               RuntimeWarning: coroutine 'Path.as_uri' was never awaited

/usr/lib/python3.13/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning
=========================== short test summary info ============================
FAILED _tests/test_path.py::test_compare_async_stat_methods[is_dir] - Attribu...
FAILED _tests/test_path.py::test_compare_async_stat_methods[is_file] - Attrib...
FAILED _tests/test_path.py::test_forward_methods_rewrap - AttributeError: wit...
FAILED _tests/test_path.py::test_forward_methods_without_rewrap - AttributeEr...
FAILED _tests/test_path.py::test_globmethods - AttributeError: write_bytes. D...
FAILED _tests/test_path.py::test_iterdir - AttributeError: write_bytes. Did y...
FAILED _tests/test_path.py::test_classmethods - AttributeError: type object '...
FAILED _tests/test_timeouts.py::test_timeouts_raise_value_error - pytest.Pyte...
@TeamSpen210
Copy link
Contributor

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.

@jakkdl
Copy link
Member

jakkdl commented Feb 2, 2024

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.

@etianen
Copy link
Contributor

etianen commented Feb 15, 2024

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.

@jakkdl
Copy link
Member

jakkdl commented Feb 15, 2024

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.

@etianen
Copy link
Contributor

etianen commented Feb 15, 2024

Seems to just be a cryptography error now:
#2955

@etianen
Copy link
Contributor

etianen commented Feb 15, 2024

Would the thing to do now be to remove the failing test matrix item and mark my PR as ready for review?

@etianen
Copy link
Contributor

etianen commented Feb 15, 2024

The fix in #2955 has been merged. Can this issue be closed now, given the remaining problems are with a 3rd-party lib?

@A5rocks A5rocks closed this as completed Feb 15, 2024
@befeleme
Copy link
Author

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.

@A5rocks A5rocks reopened this Feb 16, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Feb 16, 2024

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

@etianen
Copy link
Contributor

etianen commented Feb 16, 2024

I'll take a look at those logs later. I'm confused about why the GHA CI only had cryptography errors.

@etianen
Copy link
Contributor

etianen commented Feb 16, 2024

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.

@etianen
Copy link
Contributor

etianen commented Feb 16, 2024

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:

  • We're already enumerating all wrapped methods for type definitions.
  • The metaclass implementation is big, and only used for 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.

@TeamSpen210
Copy link
Contributor

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.

@etianen
Copy link
Contributor

etianen commented Feb 16, 2024

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:

  • Wrap call.
  • Wrap call as coroutine delegating to thread pool executor.
  • Wrap iterator.

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

@TeamSpen210
Copy link
Contributor

Well trio.Path just contains a pathlib.Path object, all we’d need to do is ensure it also accepts those subclasses.

@etianen
Copy link
Contributor

etianen commented Feb 16, 2024

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.

@jakkdl
Copy link
Member

jakkdl commented Feb 16, 2024

Current implementation is mostly @TeamSpen210's brainchild, so I think you can go ahead~

@etianen etianen mentioned this issue Feb 17, 2024
@etianen
Copy link
Contributor

etianen commented Feb 17, 2024

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.

etianen added a commit to etianen/logot that referenced this issue Mar 23, 2024
@Fuyukai
Copy link
Member

Fuyukai commented Apr 12, 2024

Closing as the PR got merged and tests seem to pass on 3.13.0a6 (at least on my machine).

@Fuyukai Fuyukai closed this as completed Apr 12, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Apr 12, 2024

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)

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

No branches or pull requests

6 participants