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

Python 3.13 compatibility #5091

Open
fedepell opened this issue Mar 24, 2024 · 15 comments
Open

Python 3.13 compatibility #5091

fedepell opened this issue Mar 24, 2024 · 15 comments

Comments

@fedepell
Copy link
Contributor

It seems that with new Python 3.13 alpha 3.13.0a5 (note with previous alpha this seemed not to happen as besides the small fix of #5035 it worked fine) tests fail very early:


+ cd robotframework-7.0
+ /usr/bin/python3 utest/run.py
Traceback (most recent call last):
  File "/builddir/build/BUILD/robotframework-7.0/utest/run.py", line 99, in <module>
    tests = get_tests(args.directory)
            ~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/robotframework-7.0/utest/run.py", line 56, in get_tests
    tests.extend(get_tests(fullname))
                 ~~~~~~~~~^^^^^^^^^^
  File "/builddir/build/BUILD/robotframework-7.0/utest/run.py", line 66, in get_tests
    module = __import__(modname)
             ~~~~~~~~~~^^^^^^^^^
  File "/builddir/build/BUILD/robotframework-7.0/utest/parsing/test_model.py", line 41, in <module>
    EXPECTED = File(sections=[
               ~~~~^^^^^^^^^^^
        ImplicitCommentSection(
        ^^^^^^^^^^^^^^^^^^^^^^^
    ...<86 lines>...
        )
        ^
    ])
    ^^
  File "/builddir/build/BUILD/robotframework-7.0/utest/../src/robot/parsing/model/blocks.py", line 72, in __init__
    super().__init__()
    ~~~~~~~~~~~~~~~~^^
SystemError: <method-wrapper '__init__' of File object at 0x7fc54cea0190> returned NULL without setting an exception

I tried to investigate a bit but sincerely got a bit lost in the flow of inheritance and what that super call should achieve. I know there is still some time for Python 3.13, but still reporting if it may give you an early warning.

If you have any proposal on a fix and need help to retest please just let me know!

@pekkaklarck pekkaklarck added this to the v7.1 milestone Mar 25, 2024
@pekkaklarck
Copy link
Member

This looks pretty strange and coulI tried reproducing this on Linux Mint, but deadsnakes only had alpha 4 and even that had issues with pip due to distutils being removed. Anyway, this looks pretty odd and could even be a bug in Python itself. That super().__init__() is there to call __init__ of ast.AST that's the root of the node hierarchy. I'm not entirely certain is calling it mandatory, at least simple tests seem to run fine also without it, but at least PyCharm complains if it's removed.

How urgent fixing this is for you? If you'd like this to be fixed in RF 7.0.1, the fix is needed pretty soon. If it's enough to get this fixed in RF 7.1 that's planned for June/July, then we have more time. One good thing with targeting RF 7.1 would be that Python 3.13 gets to beta phase in May and would then be a more stable target.

@fedepell
Copy link
Contributor Author

Thanks for checking this out! It was indeed very strange to me too being just the AST init after all.

As for urgency: please no worries! This would be needed for Fedora 41 which should be out somewhere around November, which also means by the time 3.13 should be final. Also in ~2 weeks a new 3.13 alpha should be released (and as you mention betas then in May), I can recheck there and report!

@pekkaklarck
Copy link
Member

Sounds good to me. Let's concentrate on Python 3.13 support in RF 7.1. Delayed evaluation of annotations may also require us to make changes.

@hroncok
Copy link

hroncok commented Mar 26, 2024

Indeed seems like a Python 3.13 bug. Minimal Python reproducer:

import ast


class File(ast.AST):
    _fields = ("xxx",)

    def __init__(self):
        super().__init__()


File()
$ python3.13 method_wrapper.py
Traceback (most recent call last):
  File ".../method_wrapper.py", line 11, in <module>
    File()
    ~~~~^^
  File ".../method_wrapper.py", line 8, in __init__
    super().__init__()
    ~~~~~~~~~~~~~~~~^^
SystemError: <method-wrapper '__init__' of File object at 0x7fadc9f71010> returned NULL without setting an exception

@hroncok
Copy link

hroncok commented Mar 26, 2024

I've reported python/cpython#117266

@pekkaklarck
Copy link
Member

Thanks for testing and reporting the issue @hroncok.

@pekkaklarck
Copy link
Member

If the problem is in Python itself, I guess this issue can be closed. In that case a new generic issue about Python 3.13 compatibility should be submitted. We've had similar issues with Python 3.12 (#4771) and Python 3.11 (#4401).

@JelleZijlstra
Copy link

Thanks for finding and reporting this bug in CPython this early in the release cycle!

It's possible that you will need to make changes in your code to accommodate the deprecations in https://docs.python.org/3.13/whatsnew/3.13.html#ast. I'm happy to help with that, but I looked at the file involved (https://github.com/robotframework/robotframework/blob/master/src/robot/parsing/model/statements.py) and it doesn't look like it has much to do with the Python AST. Perhaps it is enough if you add the _field_types field to your classes.

@pekkaklarck
Copy link
Member

Thanks for the info @JelleZijlstra. Do I get it right that the reported issue will be fixed in beta 1, but there's a deprecation what we need to take into account? Is the _field_types field that you mention documented somewhere? ast module docs only mentions _fields.

@JelleZijlstra
Copy link

That's right. I will have to change the docs to document _field_types.

@pekkaklarck
Copy link
Member

It's true that our ast module usage doesn't have anything to do with Python AST. We simply used it as a base when we rewrote Robot Framework's parsing model few years ago. It got us started quickly and also these days has a benefit that we can use tools like ast.dump and the external astpretty module for debugging:

>>> import astpretty
>>> from robot.api.parsing import get_model
>>> m = get_model('''\
... *** Test Cases ***
... Example
...     Log    Hello, world!
... ''')
>>> astpretty.pprint(m)
File(
    source=None,
    languages=[],
    lineno=1,
    col_offset=0,
    end_lineno=3,
    end_col_offset=25,
    errors=(),
    sections=[
        TestCaseSection(
            lineno=1,
            col_offset=0,
            end_lineno=3,
            end_col_offset=25,
            errors=(),
            header=SectionHeader(type='TESTCASE HEADER', tokens=(Token(TESTCASE_HEADER, '*** Test Cases ***', 1, 0), Token(EOL, '\n', 1, 18)), lineno=1, col_offset=0, end_lineno=1, end_col_offset=19, errors=()),
            body=[
                TestCase(
                    lineno=2,
                    col_offset=0,
                    end_lineno=3,
                    end_col_offset=25,
                    errors=(),
                    header=TestCaseName(type='TESTCASE NAME', tokens=(Token(TESTCASE_NAME, 'Example', 2, 0), Token(EOL, '\n', 2, 7)), lineno=2, col_offset=0, end_lineno=2, end_col_offset=8, errors=()),
                    body=[KeywordCall(type='KEYWORD', tokens=(Token(SEPARATOR, '    ', 3, 0), Token(KEYWORD, 'Log', 3, 4), Token(SEPARATOR, '    ', 3, 7), Token(ARGUMENT, 'Hello, world!', 3, 11), Token(EOL, '\n', 3, 24)), lineno=3, col_offset=0, end_lineno=3, end_col_offset=25, errors=())],
                ),
            ],
        ),
    ],
)

I understand that although it has worked well, the ast module isn't designed for this kind of usage. If there are changes that case bigger problems for us in the future, we can rewrite the core of our AST so that it doesn't use ast anymore.

@JelleZijlstra
Copy link

Thanks! Definitely an unusual use of AST, but since you're already doing it, from the CPython side we should preserve compatibility and continue to support it. I just opened an issue on CPython about this and I'll try to make it so that your use case continues to work well.

@pekkaklarck pekkaklarck changed the title Problem starting tests with Python 3.13.0a5 Python 3.13 compatibility Apr 17, 2024
@pekkaklarck
Copy link
Member

The original reported issue ought to be fixed in Python 3.13 itself, but we may still need to adjust our code to avoid deprecation warnings.

We obviously also want to support Python 3.13 otherwise and I changed the issue title to cover that. Supporting new Python versions hasn't typically required too much work from us, but in this case deferred evaluation of annotations (PEP 649) is likely to require at least some changed.

@JelleZijlstra
Copy link

I'm about to open a PR updating my AST changes in CPython to relax the requirements on third-party classes that inherit from ast.AST. With that PR, the robotframework test suite passes for me locally on Python 3.13 with no deprecations.

Of course, there may be other changes that land in 3.13 yet, such as PEP 649. Ideally PEP 649 would not require changes for most user code, but we can't know for sure until we have the implementation.

@pekkaklarck
Copy link
Member

Great to here ast changes will land soon. We process annotations at run time so PEP 649 certainly can affect us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants