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

feat: support for python 3.11 #31

Merged
merged 95 commits into from
Sep 20, 2022
Merged

feat: support for python 3.11 #31

merged 95 commits into from
Sep 20, 2022

Conversation

15r10nk
Copy link
Collaborator

@15r10nk 15r10nk commented Feb 7, 2022

  • This provides a new implementation, which uses co_positions() to lookup the ast node.
  • It has a simpler implementation and better performance.
  • Some limitations in the unit tests are removed for 3.11.

@alexmojaki
Copy link
Owner

Thanks! I was going to do this once 3.11 was closer, it's nice to have some help.

Please put the new logic inside NodeFinder so that the stuff about exceptions, caching, and IPython still applies.

Also please add thorough tests for 3.11. TestFiles.check_filename should now support basically all nodes.

@alexmojaki
Copy link
Owner

I get the same test failures locally using the latest 3.11 installed by pyenv, even if I go back before my changes.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Feb 8, 2022

I tested my commit with python 3.11.0a3 and it passed the tests (tox -e py311).
I will upgrade to the latest version and try to figure out what is wrong.

@alexmojaki
Copy link
Owner

Yeah they passed for me locally too (before and after my changes) until I reinstalled 3.11.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Feb 8, 2022

the generated bytecode has changed:

test script:

import dis


def foo():
    @deco1
    @deco2()
    class test:
        pass


dis.dis(foo)

python 3.11.0a3

  6           0 LOAD_GLOBAL              0 (deco1)

  7           2 LOAD_GLOBAL              1 (deco2)
              4 CALL_FUNCTION            0

  8           6 LOAD_BUILD_CLASS
              8 LOAD_CONST               1 (<code object test at 0x7f4674244e30, file "/home/frank/project
s/executing/test.py", line 6>)
             10 MAKE_FUNCTION            0
             12 LOAD_CONST               2 ('test')
             14 CALL_FUNCTION            2
             16 CALL_FUNCTION            1
             18 CALL_FUNCTION            1
             20 STORE_FAST               0 (test)
             22 LOAD_CONST               0 (None)
             24 RETURN_VALUE

Disassembly of <code object test at 0x7f4674244e30, file "/home/frank/projects/executing/test.py", line 6>
:
  6           0 LOAD_NAME                0 (__name__)
              2 STORE_NAME               1 (__module__)
              4 LOAD_CONST               0 ('foo.<locals>.test')
              6 STORE_NAME               2 (__qualname__)

  9           8 LOAD_CONST               1 (None)
             10 RETURN_VALUE

python 3.11.0a5

  4           0 RESUME                   0

  6           2 LOAD_GLOBAL              0 (deco1)

  7           4 LOAD_GLOBAL              1 (deco2)
              6 PRECALL_FUNCTION         0
              8 CALL                     0

  8          10 LOAD_BUILD_CLASS
             12 LOAD_CONST               1 (<code object test at 0x7efd0c1f4d50, file "/home/frank/project
s/executing/test.py", line 6>)
             14 MAKE_FUNCTION            0
             16 LOAD_CONST               2 ('test')
             18 PRECALL_FUNCTION         2
             20 CALL                     0

  7          22 PRECALL_FUNCTION         1
             24 CALL                     0

  6          26 PRECALL_FUNCTION         1
             28 CALL                     0

  8          30 STORE_FAST               0 (test)
             32 LOAD_CONST               0 (None)
             34 RETURN_VALUE

Disassembly of <code object test at 0x7efd0c1f4d50, file "/home/frank/projects/executing/test.py", line 6>
:
  6           0 RESUME                   0
              2 LOAD_NAME                0 (__name__)
              4 STORE_NAME               1 (__module__)
              6 LOAD_CONST               0 ('foo.<locals>.test')
              8 STORE_NAME               2 (__qualname__)

  9          10 LOAD_CONST               1 (None)
             12 RETURN_VALUE

What was previously a CALL_FUNCTION is now PRECALL_FUNCTION & CALL.
It looks like the source range of this function calls refers to the ast nodes of the decorators.
I will look deeper into it.

But it is questionable if it makes sense, because this might change again till the release of 3.11.

@alexmojaki
Copy link
Owner

I'd be happy with a first PR that:

  • Fixes the main functionality, so Executing.node has the right value. This affects the most people by far and so far no problems obstacles for it have been identified.
  • Doesn't try to identify the decorator in 3.11 at all, to make sure it doesn't silently identify the wrong one. The decorator functionality matters very little, and as you say the bytecode might change.
  • Skips parts of the tests for 3.11:
    • test_decorator
    • Parts of test_files:
      • Checking decorators
      • Checking code qualnames since find_qualnames also doesn't work any more. The actual qualname functionality still seems fine, find_qualnames is a trick for the sake of tests to find the qualname in a different way.

Followup PRs to fix decorators and find_qualnames would still be appreciated, and might make sense to do soon if an easy way can be found, but they're not worth spending a lot of effort on until 3.11 is closer to final release.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Feb 8, 2022

I was able to detect the decorator, but that is enough for today.
I will take a look at NodeFinder TestFiles.check_filename tomorrow.

@frenzymadness
Copy link
Contributor

We are rebuilding all RPM packages in Fedora with the new Python 3.11. I can confirm that the problem is still there with Python 3.11a6 with this patch applied. The result is:

+ /usr/bin/python3 -m tox --current-env -q --recreate -e py311
s.....F................
======================================================================
FAIL: test_decorator (__main__.TestStuff)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/executing-0.8.2/tests/test_main.py", line 60, in test_decorator
    @tester(list(tuple([7, 8])))  # 7!
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/executing-0.8.2/tests/utils.py", line 46, in __call__
    self.check(call.args[0], arg)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/executing-0.8.2/tests/utils.py", line 37, in check
    assert result == value, (result, value)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: ([7, 8], <function TestStuff.test_decorator.<locals>.foo at 0x7f7b5b437f60>)

----------------------------------------------------------------------
Ran 23 tests in 0.989s

FAILED (failures=1, skipped=1)
0.5
ERROR: InvocationError for command /builddir/build/BUILD/executing-0.8.2/.tox/py311/bin/python tests/test_main.py (exited with code 1)
___________________________________ summary ____________________________________

@alexmojaki
Copy link
Owner

@15r10nk just reiterating that the decorator and qualnames are low priority.

@frenzymadness
Copy link
Contributor

The relevant part of Python 3.11 changelog: https://docs.python.org/3.11/whatsnew/3.11.html#cpython-bytecode-changes

@15r10nk
Copy link
Collaborator Author

15r10nk commented Mar 31, 2022

I took a look at it and there are new CACHE instructions added, which change the current handling of decorators.

I also have problems to understand some of the test cases (the long running especially) and I am not sure which I can ignore.

@alexmojaki
Copy link
Owner

I took a look at it and there are new CACHE instructions added, which change the current handling of decorators.

Then skip or xfail the failing decorator tests in 3.11.

I also have problems to understand some of the test cases (the long running especially) and I am not sure which I can ignore.

Skip the failing self.assertEqual(code_qualname, qualname) in 3.11.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Mar 31, 2022

Decorators should work again (was not so difficult in the end),
but the long running tests are still failing.

env EXECUTING_SLOW_TESTS=1 pyenv exec python -m unittest tests.test_main
fails.

@alexmojaki
Copy link
Owner

The log is saying that it failed to find a node for this instruction:

Instruction(opname='LOAD_METHOD', opcode=160, arg=13, argval='start', argrepr='start', offset=302, starts_line=105, is_jump_target=False, positions=Positions(lineno=95, end_lineno=95, col_offset=30, end_col_offset=39))

That's the .start in this code in tests/samples/ipython.py:

    @cell_magic
    def eye(self, _line, cell):
        if not self.server_url:
            server.db = Database(self.db_url)
            server.Function = server.db.Function
            server.Call = server.db.Call
            server.Session = server.db.Session
            Thread(
                target=run_server,
                args=(
                    self.port,
                    self.bind_host,
                    self.show_server_output,
                ),
            ).start()

Everything seems to agree correctly that that's on line 105, except for the instruction positions which seems to correspond to server.db. Looks like a bug in 3.11 to me.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Mar 31, 2022

Your error looks also like a bug in 3.11 for me, but I don't know if we get the same errors.

My looks like this:
env EXECUTING_SLOW_TESTS=1 pyenv exec python -m unittest tests.test_main

/home/frank/projects/executing/tests/samples/bird.py
... a lot of source code ...
---
standard_library.install_aliases()
5
0
False
[]

If I understand the test correctly the problem is that it does not find any bytecode instruction for this ast node.
Maybe my problem is caused by the same bug than yours.

@alexmojaki
Copy link
Owner

I'm looking at the CI logs here, I didn't run it myself. But yes, the sample files were probably just listed in different orders, if you checked the ipython sample first it would probably give you the same error.

If I understand the test correctly the problem is that it does not find any bytecode instruction for this ast node.

That's correct. Look for places where it checks that the name starts with CALL_ and change it to CALL. The changelog link posted above mentions that as a new instruction.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Mar 31, 2022

I was able to fix the line number mismatch:

Instruction(opname='LOAD_METHOD', opcode=160, arg=13, argval='start', argrepr='start', offset=318, starts_line=105, is_jump_target=False, positions=Positions(lineno=105, end_lineno=105, col_offset=12, end_col_offs et=21))

However I don't understand why my change fixes the problem ...

And it seems that the line numbers are not the actual problem.

I suspect that the positions in the instructions are not taken directly from AST ranges (which would break my approach).

I don't know if this is intentional or a bug in python.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Apr 1, 2022

python/cpython@d48848c83e this commit changed the line number for the LOAD_METHOD instruction (file compile.c function maybe_optimize_method_call).
This behaviour is intended to show the correct source range for attribute access.

That is the reason why I am not able to find the ast node and the only(...) call fails in the test.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Apr 9, 2022

Created this two cpython issues which are related to this:
https://bugs.python.org/issue47253
https://bugs.python.org/issue47233

@frenzymadness
Copy link
Contributor

Just a note that the first beta is out and no new features should land in Python 3.11.

@hrnciar
Copy link

hrnciar commented Jun 1, 2022

Hello, 2nd beta of Python 3.11 is out. When I tried to build executing with this PR the test_decorator failed with:

======================================================================
FAIL: test_decorator (__main__.TestStuff.test_decorator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/executing-0.8.2/tests/test_main.py", line 60, in test_decorator
    @tester(list(tuple([7, 8])))  # 7!
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/executing-0.8.2/tests/utils.py", line 46, in __call__
    self.check(call.args[0], arg)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/executing-0.8.2/tests/utils.py", line 37, in check
    assert result == value, (result, value)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: ([7, 8], <function TestStuff.test_decorator.<locals>.foo at 0x7f2902199d00>)

----------------------------------------------------------------------
Ran 23 tests in 1.149s

FAILED (failures=1, skipped=1)
0.5
ERROR: InvocationError for command /builddir/build/BUILD/executing-0.8.2/.tox/py311/bin/python tests/test_main.py (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   py311: commands failed

@aroberge
Copy link

I was wondering if it would be possible to do a partial merge that does not include a fix for the decorators, as suggested earlier. My reason is that I have currently many tests that I must skip in friendly-traceback for Python 3.11 and I would like to figure out if these will work with the proposed fix, or if they require further changes to executing.

15r10nk and others added 6 commits July 16, 2022 16:12
* This provides a new implementation, which uses co_positions() to lookup the ast node.
* It has a simpler implementation and better performance.
* Some limitations in the unit tests are removed for 3.11.
    * support for `and` and `or`
    * no ambiguities for generators
@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 14, 2022

I think I fixed all the issues you mentioned. Let me know if I forgot something.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Sep 17, 2022

I let it run over 428514 python files to find the last issues. I think it is getting pretty stable.

tox.ini Outdated Show resolved Hide resolved
Copy link
Owner

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to install .[tests] in .github/...yml instead of the manually listed requirements.

setup.cfg Outdated Show resolved Hide resolved
@alexmojaki
Copy link
Owner

Huge thank you for this massive contribution! Merging now but going to test it with other packages before I release.

@alexmojaki alexmojaki merged commit 589d798 into alexmojaki:master Sep 20, 2022
This was referenced Sep 27, 2022
@alexmojaki alexmojaki mentioned this pull request May 8, 2024
9 tasks
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

Successfully merging this pull request may close these issues.

Support for Python 3.11?
5 participants