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

[BUG] PyPy-specific compiler crash in a function with fused types #5588

Closed
althonos opened this issue Aug 4, 2023 · 8 comments
Closed

[BUG] PyPy-specific compiler crash in a function with fused types #5588

althonos opened this issue Aug 4, 2023 · 8 comments

Comments

@althonos
Copy link

althonos commented Aug 4, 2023

Describe the bug

When compiling a Cython class cpdef method with fused type arguments and default arguments, I get a compiler crash (see next section). This happens only when running Cython with pypy (PyPy 7.3), the same code compiles fine with CPython (Python 3.10). The error seems to originate from Cython.COmpiler.PyrexTypes, as shown in the traceback.

Code to reproduce the behaviour:

I have a hard time reproducing it because this comes from a larger codebase (althonos/pyhmmer) but I have the following method inside a cdef class:

cpdef TopHits search_msa(self, DigitalMSA query, SearchTargets sequences, Builder builder = None)

where TopHits, DigitalMSA and Builder are Cython classes, and SearchTargets a fused type.

I'm getting the following crash, but only when compiling with PyPy, with CPython it compiles fines:

pyhmmer/plan7.pyx:5589:10: Compiler crash in AnalyseDeclarationsTransform

ModuleNode.body = StatListNode(plan7.pyx:15:0)
StatListNode.stats[116] = StatListNode(plan7.pyx:4763:5)
StatListNode.stats[0] = CClassDefNode(plan7.pyx:4763:5,
    as_name = 'Pipeline',
    class_name = 'Pipeline',
    doc = '...',
    module_name = '',
    punycode_class_name = 'Pipeline',
    visibility = 'private')
CClassDefNode.body = StatListNode(plan7.pyx:4764:4)
StatListNode.stats[29] = CFuncDefNode(plan7.pyx:5589:10,
    args = [...]/4,
    doc = '...',
    has_fused_arguments = True,
    is_c_class_method = 1,
    modifiers = [...]/0,
    outer_attrs = [...]/2,
    overridable = 1,
    visibility = 'private')
File 'FusedNode.py', line 62, in __init__: FusedCFuncDefNode(plan7.pyx:5589:10,
    fused_compound_types = [...]/1,
    match = "dest_sig[{{dest_sig_idx}}] = '{{specialized_type_name}}'",
    no_match = 'dest_sig[{{dest_sig_idx}}] = None',
    nodes = [...]/2)

Compiler crash traceback from this point on:
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Compiler/FusedNode.py", line 62, in __init__
    self.copy_cdef(env)
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Compiler/Symtab.py", line 259, in __repr__
    return "%s(<%x>, name=%s, type=%s)" % (type(self).__name__, id(self), self.name, self.type)
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Compiler/PyrexTypes.py", line 301, in __str__
    return self.declaration_code("", for_display = 1).strip()
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Compiler/PyrexTypes.py", line 3258, in declaration_code
    arg_decl_list.append(self.op_arg_struct.declaration_code(Naming.optional_args_cname))
AttributeError: 'NoneType' object has no attribute 'declaration_code'
Traceback (most recent call last):
  File "/home/althonos/Code/pyhmmer/setup.py", line 740, in <module>
    setuptools.setup(
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/__init__.py", line 107, in setup
    return distutils.core.setup(**attrs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/_distutils/core.py", line 185, in setup
    return run_commands(dist)
           ^^^^^^^^^^^^^^^^^^
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/_distutils/core.py", line 201, in run_commands
    dist.run_commands()
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/_distutils/dist.py", line 969, in run_commands
    self.run_command(cmd)
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/dist.py", line 1234, in run_command
    super().run_command(command)
  File "/home/althonos/.local/lib/pypy3.10/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
    cmd_obj.run()
  File "/home/althonos/Code/pyhmmer/setup.py", line 133, in run
    self.extensions = cythonize(self.extensions, **cython_args)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Build/Dependencies.py", line 1134, in cythonize
    cythonize_one(*args)
  File "/home/althonos/.local/lib/pypy3.10/site-packages/Cython/Build/Dependencies.py", line 1301, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: pyhmmer/plan7.pyx

Expected behaviour

The Cython code (and the generated C code) compiles and works.

OS

Linux

Python version

Python 3.10.12 [PyPy 7.3.12 with GCC 13.1.1 20230429]

Cython version

3.0.0

Additional context

I actually tried modifying PyrexTypes.py:

if self.optional_arg_count:
arg_decl_list.append(self.op_arg_struct.declaration_code(Naming.optional_args_cname))

To check that self.op_arg_struct is not none:

if self.optional_arg_count and self.op_arg_struct is not None:
    arg_decl_list.append(self.op_arg_struct.declaration_code(Naming.optional_args_cname))

In that case I can compile both the Cython file and the resulting C code succesfully, but maybe that's just a hotfix and it breaks things elswehere 🤷 .

@mattip
Copy link
Contributor

mattip commented Aug 4, 2023

I wonder if there is some difference in the function signature introspection between CPython and PyPy that leads to the missing op_arg_struct.

@da-woods
Copy link
Contributor

da-woods commented Aug 4, 2023

One thing I wonder (without having done any investigation or tried anything): is this a difference between "Cython compiled to extension modules" and "Cython run as Python code" rather than a difference between CPython and PyPy.

That'd seem more likely to me. Because at the point you hit this bug, the generated code should be completely independent of the Python version used to generate it.

@althonos
Copy link
Author

althonos commented Aug 4, 2023

I mean, I have Cython 3.0 installed for both CPython and PyPy; when I try to compile the same code (either with python setup.py build_ext --inplace or with pypy3 setup.py build_ext --inplace, both of which internally call Cython.Build.cythonize) I get the bug above with pypy3 but not with python. That happened in CI as well, where only the PyPy builds were crashing.

@da-woods
Copy link
Contributor

da-woods commented Aug 4, 2023

Just to explain my comment: the Cython wheels that we distribute for CPython are typically compiled with Cython themselves. The versions we distribute for PyPy typically aren't. I'm guessing that this is more likely to be the difference than PyPy itself.

It needs proper investigation though (and not asking you to do the investigation) - that is just a guess at this stage.

@scoder
Copy link
Contributor

scoder commented Aug 5, 2023 via email

@althonos
Copy link
Author

althonos commented Aug 5, 2023

To try it out, you can download the pure Python wheel of Cython from PyPI and install it on CPython, then see if the build fails there, too. (There's also a pip option to avoid binary wheels, but I can't look that up right now.)

I tried this, and the CPython build was fine.

Additionally, I was able to confirm that this crash is only for PyPy 3.9 and prior, I'm not getting it with PyPy 3.10.

@da-woods
Copy link
Contributor

da-woods commented Aug 6, 2023

I've at least got it down to a short reproducible example

something.pxd

ctypedef fused f1:
    int
    double

cdef class C:
    cpdef object xxx(self, f1 x, a=?)

something.pyx

cdef class C:
    cpdef object xxx(self, f1 x, a=None):
        return x+a

def call(C c, double x):
    c.xxx(x)

and just build with PYTHON cythonize.py -if something.pyx where PYTHON is either pypy or CPython

@da-woods
Copy link
Contributor

da-woods commented Aug 6, 2023

PyPy bug: https://foss.heptapod.net/pypy/pypy/-/issues/3978

try:
cindex = env.cfunc_entries.index(self.node.entry)
except ValueError:
env.cfunc_entries.extend(new_cfunc_entries)
else:
env.cfunc_entries[cindex:cindex+1] = new_cfunc_entries

self.node.entry isn't in a state where it can be printed. On CPython this doesn't matter. On PyPy it causes a failure while preparing the ValueError and thus you get a different exception.

I'm going to leave this open here because what we have in Cython feels fragile. Maybe we should make it so that CFuncType.declaration_code can succeed (at least in for_display mode) even when op_arg_struct is None? Because being able to print things is useful. So I think there's useful cleanup to be done in Cython even if the bug isn't a Cython bug

da-woods added a commit to da-woods/cython that referenced this issue Dec 5, 2023
In some Python versions, generating the error message when .index
fails to find an index leads to a compiler crash.

Fix this by not relying on type being fully set up while generating
__str__.

Fixes cython#5894 and cython#5588
da-woods added a commit that referenced this issue Dec 8, 2023
* Fix an issue trying list.index indexing in FusedNode

In some Python versions, generating the error message when .index
fails to find an index leads to a compiler crash.

Fix this by not relying on type being fully set up while generating
__str__.

Fixes #5894 and #5588

* Check "in" before indexing
da-woods added a commit that referenced this issue Dec 8, 2023
* Fix an issue trying list.index indexing in FusedNode

In some Python versions, generating the error message when .index
fails to find an index leads to a compiler crash.

Fix this by not relying on type being fully set up while generating
__str__.

Fixes #5894 and #5588

* Check "in" before indexing
da-woods added a commit that referenced this issue Dec 8, 2023
* Fix an issue trying list.index indexing in FusedNode

In some Python versions, generating the error message when .index
fails to find an index leads to a compiler crash.

Fix this by not relying on type being fully set up while generating
__str__.

Fixes #5894 and #5588

* Check "in" before indexing
@da-woods da-woods added the defect label Dec 8, 2023
@da-woods da-woods closed this as completed Dec 8, 2023
@da-woods da-woods added this to the 0.29.37 milestone Dec 8, 2023
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