Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#22305 closed enhancement (fixed)

weak_dict.pyx is not python3 compatible

Reported by: aapitzsch Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords:
Cc: Jeroen Demeyer, Frédéric Chapoton Merged in:
Authors: Nils Bruin Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 54d8c2d (Commits, GitHub, GitLab) Commit:
Dependencies: #22942 Stopgaps:

Status badges

Description (last modified by Frédéric Chapoton)

The structure of PyDictObject used by weak_dict changed from Python 2 to Python 3, e.g PyDictEntry has been removed

REF:

Attachments (1)

weak_dict.pyx (43.6 KB) - added by Nils Bruin 5 years ago.
Python3.6 compatible version of weak_dict.pyx

Download all attachments as: .zip

Change History (53)

comment:1 Changed 6 years ago by Nils Bruin

The change is significant. CPython3 still uses the same underlying logic, so that should still work, but it looks like it will be difficult to support both Py2 and Py3 with the same source. Do we have conditional compilation flags in cython for such cases? I imagine the transition is smoothest if we can ensure that we can support Py2 and Py3 from a single source, at least for a while.

If I recall correctly: PyDict_GetItemWithError is available in Py3.

For the rest, it should just be del_dictitem_by_exact_value that needs rewriting to account for the renaming of certain structures and, most importantly, dealing with split tables (which is new) as well as combined tables (which are basically the old dict lay-out).

It may be worth checking if the special trick used to participate in the Python Trashcan is still necessary with Py3.

Iteration protection has changed in Py3 as well a bit, so perhaps we don't quite need the protection we're enforcing in deletion anymore.

I think presently the "contains" test also reaches into the internal structure. However, "contains" doesn't really produce verifiable results on a WeakValueDict anyway, because the member can always disappear between the containment test and retrieval, so we could not override that routine (in the same way that "len" is not overridden).

For me the main issues that prevent me from fixing this issue are that I don't have access to a platform where I can compile cython against python 3 and that I don't know how to make cython source compile conditionally wrt. Py2 vs. Py3. It's probably easier if someone who does know how to deal with those things makes the necessary adjustments. It's not rocket science, but you do have to carefully read dictobject.c

If we could somehow link to delitem_common the deletion would almost be taken care of. I'm afraid that's not an exported symbol, though, so we basically have to replicate it.

Version 2, edited 6 years ago by Nils Bruin (previous) (next) (diff)

comment:2 Changed 6 years ago by Frédéric Chapoton

build failure looks like that:

[sagelib-7.6.beta4] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c: In function '__pyx_f_4sage_4misc_9weak_dict_PyDict_GetItemWithError':
[sagelib-7.6.beta4] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1393:3: error: unknown type name 'PyDictEntry'
[sagelib-7.6.beta4]    PyDictEntry *__pyx_v_ep;
[sagelib-7.6.beta4]    ^
[sagelib-7.6.beta4] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1398:3: error: unknown type name 'PyDictEntry'
[sagelib-7.6.beta4]    PyDictEntry *__pyx_t_2;
[sagelib-7.6.beta4]    ^
[sagelib-7.6.beta4] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1419:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_lookup'

comment:3 Changed 6 years ago by Frédéric Chapoton

Description: modified (diff)

comment:4 Changed 6 years ago by Frédéric Chapoton

Type: PLEASE CHANGEenhancement

comment:5 Changed 6 years ago by Frédéric Chapoton

Cc: Jeroen Demeyer added

This is one of our cython+python3 compilation problems.

comment:6 Changed 6 years ago by Frédéric Chapoton

Milestone: sage-7.6sage-8.0

comment:7 Changed 5 years ago by Frédéric Chapoton

Could somebody do some action on the problem here, please ?

comment:8 Changed 5 years ago by Frédéric Chapoton

This is probably the last cython file that does not compile with python3 !!

comment:9 Changed 5 years ago by Nils Bruin

Is there an easy way to get a "cython+python3" setup on which I can try weak_dict? I do recall how I made it work on py2, so I might be in a reasonable position to adapt it to py3.

Do we need a way to switch between the two? I don't think it will be so easy to make a version that works on both py2 and py3 (given that we reach into dicts beyond what's exposed by the API)

comment:10 Changed 5 years ago by Nils Bruin

OK, I've made some headway here. The changes are quite manageable. However, we do need the content and memory layout of the PyDictKeysObject. That's not part of the API and the layout of the PyDictKeysObject struct isn't even exported in Python.h (it's in dict-common.h, which is an internal, non-exported header file in CPython.).

As before, this means that this module might need rewriting if CPython decides to change the memory layout (and/or its hash probing).

The bad news is that they have done exactly that from 3.5 to 3.6 (they added order-preserving properties to dictionaries). So: do we have a particular target we're aiming for with sage? 3.5 vs. 3.6 makes a significant difference (larger than 2.7 vs. 3.5 actually)

Of course, reaching into CPython beyond the official CAPI is questionable. The alternative is to reimplement the entirety of dict in order to have WeakValueDictionary. Either that or try to get this thing accepted up-stream (after all, the WeakValueDictionary that ships with python (also the one with python 3) has demonstrable problems). Are we comfortable with the added maintenance burden of a module that depends on more than just the CAPI of CPython?

Last edited 5 years ago by Nils Bruin (previous) (diff)

comment:11 Changed 5 years ago by Nils Bruin

OK, I've attached a version of weak_dict.pyx that compiles on python 3.6 and that seems to work too. Some more testing should be done and someone should look into how we can merge this into sage without breaking it on python 2.7

Changed 5 years ago by Nils Bruin

Attachment: weak_dict.pyx added

Python3.6 compatible version of weak_dict.pyx

comment:12 Changed 5 years ago by Nils Bruin

Some updates. Main thing is that the default lookup function doesn't expect deleted entries, so we should trigger the dict to use the more general lookup.

There is a dictionary variant that has "split tables" (for keys and values, so that it can share the keys between different instances). These should only occur as instance dictionaries of objects so they shouldn't occur for WeakValueDictionaries. These don't support deletions either.

comment:13 Changed 5 years ago by Nils Bruin

Cc: Frédéric Chapoton added

Ping to chapoton. Did you need a fix urgently? The file attached here does solve the compilation issue on Python3.6.

comment:14 Changed 5 years ago by Frédéric Chapoton

i have no available time this week

comment:15 Changed 5 years ago by Frédéric Chapoton

Branch: public/22305
Commit: 02a2b13df369e26bb463c6bf2429c7873a4f972c

I made a git branch with your latest patch.


New commits:

02a2b13py3.6 compatible weak dictionaries

comment:16 Changed 5 years ago by git

Commit: 02a2b13df369e26bb463c6bf2429c7873a4f972c787975ecdfe248586d6bafcfe407d566096e24bc

Branch pushed to git repo; I updated commit sha1. New commits:

787975etrac 22305 adding the new weak_dict.pyx

comment:17 Changed 5 years ago by Frédéric Chapoton

I made a git branch with your latest patch.

EDIT: it was not cythonizing correctly because of a typo. Now corrected in latest commit.

Last edited 5 years ago by Frédéric Chapoton (previous) (diff)

comment:18 Changed 5 years ago by git

Commit: 787975ecdfe248586d6bafcfe407d566096e24bc8722eb7a8fbbef4b6bdc94f19a7a69ea1e238cbd

Branch pushed to git repo; I updated commit sha1. New commits:

8722eb7trac 22305 typo

comment:19 Changed 5 years ago by Frédéric Chapoton

next step here would be to have the old code for python2 and the new for python3. I have no idea how to do that.

comment:20 in reply to:  19 Changed 5 years ago by Nils Bruin

Replying to chapoton:

next step here would be to have the old code for python2 and the new for python3. I have no idea how to do that.

Yes, I looked into that and came up with nothing either. I got the impression that the setup.py file would be the place to set a flag depending on the target, and then use "IF" in cython to do a conditional compile. We could separate out the code for delete_by_exact_value into a separate file if that makes things a little cleaner. That's the only routine that depends on the implementation details of dict.

It looks like the Cython IF is a little more restricted that the C preprocessor one. I tried IF PY_HEX_VERSION > 0x03060000: but couldn't get it to work.

comment:21 Changed 5 years ago by Frédéric Chapoton

@jdemeyer, do you know how to achieve properly this kind of py2/py3 switch in cython code ?

comment:22 Changed 5 years ago by Jeroen Demeyer

I'll have a look.

comment:23 Changed 5 years ago by Jeroen Demeyer

Besides: copying all that stuff from Python's sources is really bad style. I would much prefer to patch Python to make whatever you need public.

comment:24 Changed 5 years ago by Jeroen Demeyer

I am not convinced that we need completely separate files for Python 2 and Python 3. There would be a lot of code duplication and that is bad style.

Some changes for Python 3 already make sense in Python 2 (just to give one example: replacing long by Py_hash_t).

comment:25 Changed 5 years ago by Jeroen Demeyer

My proposal would be:

  1. Make a separate ticket to deal with changes to weak_dict.pyx which are good to do anyway regardless of Python vesion.
  1. Patch Python 3 to make public whatever is needed for the Python 3 version of weak_dict.pyx.
  1. Finally, use conditional compilation to deal with Python 2/3 differences in weak_dict.pyx.

comment:26 Changed 5 years ago by Nils Bruin

Number 2 is a useful exercise in trying to see if we can get a patch submitted upstream, but realistically I don't think it's very realistic that it will be accepted, given that "deleting an item from a dict by hash and exact value" isn't something that fits in the dict data abstraction very well, and might be hard to realize on other implementations than CPython.

Requiring a patched python 3 for sage would really be a step backwards in terms of making sage available via other means than its own distribution packaging, so I don't think we should do that.

I agree we can split out differences quite a bit. Basically, it's just the implementation of delete_by_exact_value that will depend on the python version, so we could park that in a separate file at least. We're going to need to differentiate *some* cython compilation on python version, though, and currently I don't know how to achieve that. (that's point 3). Can we clear that up? It shouldn't be necessary to program this stuff directly in C, just to have access to PY_HEX_VERSION ...

comment:27 Changed 5 years ago by Frédéric Chapoton

ping ? Jeroen, if you want to make a ticket for your point 1, please do so.

comment:28 Changed 5 years ago by Nils Bruin

The mechanism to pass compile-time variables to cython (for use in IF ...) seems to be something along the lines of:

from distutils.core import setup
from Cython.Build import cythonize
from sys import version_info

if version_info.major <= 2:
    env = {'PYTHON_VERSION': 2}
elif version_info.major == 3 and version_info.minor >= 6:
    env = {'PYTHON_VERSION': 3}
else:
    raise RuntimeError("Unsupported version")

setup(
    name = 'weak_dict',
    ext_modules = cythonize("weak_dict.pyx",compile_time_env=env),
)

I have no idea where put this in sage, though. I haven't been able to find the "setup" command that is relevant for "weak_dict".

comment:29 Changed 5 years ago by git

Commit: 8722eb7a8fbbef4b6bdc94f19a7a69ea1e238cbd4c73dbec8a910509aa837233f878099e62a379ed

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7b4ec6cAdd sys.version_info as "PYTHON_VERSION" to cython compile-time environment
4c73dbetract #22305 -- split off del_dictitem_by_exact_value and comile conditionally based on Py2/Py3

comment:30 Changed 5 years ago by git

Commit: 4c73dbec8a910509aa837233f878099e62a379ed71b68d22215182fa7700ddb73ecd1bf82fda84f4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

71b68d2tract #22305 -- split off del_dictitem_by_exact_value and compile conditionally based on Py2/Py3

comment:31 Changed 5 years ago by Nils Bruin

Authors: Nils Bruin
Status: newneeds_review

It seems we can simply add sys.hexversion as PY_VERSION_HEX (which is the CAPI name of the variable) to the compile-time environment for all sage cythonization. That should be fairly low-cost.

I have split off the python-version-sensitive code into a separate file, which seems to work fine. Code replication is now a lot more limited. If someone sees an economic way of sharing the docstring, that would be great. On the other hand, these two implementations are so different that having different docs (and tests?) for them might make sense.

I have tested compiling it within sage on Py2.7 and directly on Python3.6 via the following setup.py:

from distutils.core import setup
from Cython.Build import cythonize
import sys

setup(
    name = 'weak_dict',
    ext_modules = cythonize(["weak_dict.pyx","dict_del_by_value.pyx"],compile_time_env={'PY_VERSION_HEX':sys.hexversion}),
)

Since this routine breaks the abstraction of "PyDict?" I am pretty sure the code would not get accepted as an API change. The only thing that I could see happening is that python might accept the routine as an internal one and put WeakValueDict in C to use it.

The only option I see to do this "cleanly" outside of Python is to implement our own WeakValueDict from scratch, i.e., don't base it on dict. I don't think that is less maintenance burden than tracking python's dict, and certainly a larger change than the one on this ticket at this point in sage.

comment:32 Changed 5 years ago by git

Commit: 71b68d22215182fa7700ddb73ecd1bf82fda84f454d8c2da8ad19e610fe9939b90bc23202c5b5525

Branch pushed to git repo; I updated commit sha1. New commits:

54d8c2dCorrection: Py3 does provide PyDict_GetItemWithError but Cython doesn't provide it yet

comment:33 Changed 5 years ago by Frédéric Chapoton

seems to be ok on python2 sage, see patchbot report

I will try with SAGE_PYTHON3=yes

comment:34 Changed 5 years ago by Frédéric Chapoton

hum, it seems that latest update to numpy broke the SAGE_PYTHON3 build..

comment:35 Changed 5 years ago by John Palmieri

See #22582 for a numpy upgrade which should fix the python 3 build.

comment:36 Changed 5 years ago by Frédéric Chapoton

a tentative python3-compilation answers the following:

cdef del_dictitem_by_exact_value(PyDictObject *mp, PyObject *value, Py_hash_t ha
sh)
                                ^
------------------------------------------------------------

sage/misc/dict_del_by_value.pxd:5:33: Non-extern C function 'del_dictitem_by_exa
ct_value' declared but not defined

comment:37 Changed 5 years ago by Nils Bruin

Was that on <=python3.5 ? Presently the implementation of dictionaries in CPython 3.0 -- 3.5 is not supported, and as a result dict_del_by_value.pyx dutifully refrains from defining a routine.

comment:38 Changed 5 years ago by Frédéric Chapoton

Yes, this is on the current python3 in sage, which is 3.5.1

So this means that we have to upgrade python3 package, right ?

comment:39 in reply to:  38 Changed 5 years ago by Nils Bruin

Replying to chapoton:

Yes, this is on the current python3 in sage, which is 3.5.1

So this means that we have to upgrade python3 package, right ?

Yes. I am surprised your earlier experiments worked then. If the code that was here before even compiled against 3.5, it would certainly segfault.

https://docs.python.org/3/whatsnew/3.6.html#whatsnew36-compactdict

comment:40 Changed 5 years ago by Frédéric Chapoton

I have created #22942

comment:41 Changed 5 years ago by Frédéric Chapoton

Dependencies: #22942

comment:42 Changed 5 years ago by Frédéric Chapoton

Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_review

ok, green bot, I am setting to positive.

comment:43 Changed 5 years ago by Frédéric Chapoton

Status: positive_reviewneeds_work

hmm, does not seem to compile with SAGE_PYTHON3=yes and python3.6.1

comment:44 Changed 5 years ago by Frédéric Chapoton

log:

[sagelib-8.0.beta7] [1/1] gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/chapoton/sage3/local/include -I/home/chapoton/sage3/local/include/python3.6m -I/home/chapoton/sage3/local/lib/python3.6/site-packages/numpy/core/include -I/home/chapoton/sage3/src -I/home/chapoton/sage3/src/sage/ext -I/home/chapoton/sage3/src/build/cythonized -I/home/chapoton/sage3/src/build/cythonized/sage/ext -I/home/chapoton/sage3/local/include/python3.6m -c /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c -o build/temp.linux-x86_64-3.6/home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.o -fno-strict-aliasing
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c: In function '__pyx_f_4sage_4misc_9weak_dict_PyDict_GetItemWithError':
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1398:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1403:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_t_2;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1424:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_lookup'
[sagelib-8.0.beta7]    __pyx_t_2 = __pyx_v_mp->ma_lookup(__pyx_v_mp, ((PyObject *)((void *)__pyx_v_key)), __pyx_t_1); if (unlikely(__pyx_t_2 == NULL)) __PYX_ERR(0, 152, __pyx_L1_error)
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1444:25: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]      __pyx_r = __pyx_v_ep->me_value;
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c: In function '__pyx_f_4sage_4misc_9weak_dict_init_dummy':
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1497:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep0;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1498:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1503:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_t_2;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1538:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_table'
[sagelib-8.0.beta7]    __pyx_t_2 = __pyx_v_mp->ma_table;
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1558:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_mask'
[sagelib-8.0.beta7]    __pyx_t_3 = __pyx_v_mp->ma_mask;
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1588:29: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]      __pyx_t_6 = ((__pyx_v_ep->me_key != NULL) != 0);
[sagelib-8.0.beta7]                              ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1598:27: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]        __pyx_r = __pyx_v_ep->me_key;
[sagelib-8.0.beta7]                            ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c: In function '__pyx_f_4sage_4misc_9weak_dict_del_dictitem_by_exact_value':
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1655:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep0;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1656:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1660:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_t_1;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1674:37: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_mask'
[sagelib-8.0.beta7]    __pyx_v_mask = ((size_t)__pyx_v_mp->ma_mask);
[sagelib-8.0.beta7]                                      ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1683:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_table'
[sagelib-8.0.beta7]    __pyx_t_1 = __pyx_v_mp->ma_table;
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1711:27: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_t_2 = ((__pyx_v_ep->me_key == NULL) != 0);
[sagelib-8.0.beta7]                            ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1751:42: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]      __pyx_t_3 = ((((PyObject *)__pyx_v_ep->me_value) != __pyx_v_value) != 0);
[sagelib-8.0.beta7]                                           ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1757:29: error: request for member 'me_hash' in something not a structure or union
[sagelib-8.0.beta7]      __pyx_t_3 = ((__pyx_v_ep->me_hash != __pyx_v_hash) != 0);
[sagelib-8.0.beta7]                              ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1787:29: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]      __pyx_t_2 = ((__pyx_v_ep->me_key == NULL) != 0);
[sagelib-8.0.beta7]                              ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1839:54: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_t_5 = PyList_SetItem(__pyx_v_T, 0, __pyx_v_ep->me_key); if (unlikely(__pyx_t_5 == -1)) __PYX_ERR(0, 258, __pyx_L1_error)
[sagelib-8.0.beta7]                                                       ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1889:13: error: request for member 'me_key' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_v_ep->me_key = __pyx_v_4sage_4misc_9weak_dict_dummy;
[sagelib-8.0.beta7]              ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1898:54: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_t_5 = PyList_SetItem(__pyx_v_T, 1, __pyx_v_ep->me_value); if (unlikely(__pyx_t_5 == -1)) __PYX_ERR(0, 263, __pyx_L1_error)
[sagelib-8.0.beta7]                                                       ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:1907:13: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_v_ep->me_value = NULL;
[sagelib-8.0.beta7]              ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c: In function '__pyx_pf_4sage_4misc_9weak_dict_19WeakValueDictionary_22__contains__':
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:4290:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_v_ep;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:4294:3: error: unknown type name 'PyDictEntry'
[sagelib-8.0.beta7]    PyDictEntry *__pyx_t_2;
[sagelib-8.0.beta7]    ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:4316:25: error: 'PyDictObject {aka struct <anonymous>}' has no member named 'ma_lookup'
[sagelib-8.0.beta7]    __pyx_t_2 = __pyx_v_mp->ma_lookup(__pyx_v_mp, ((PyObject *)((void *)__pyx_v_k)), __pyx_t_1); if (unlikely(__pyx_t_2 == NULL)) __PYX_ERR(0, 928, __pyx_L1_error)
[sagelib-8.0.beta7]                          ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:4326:26: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_t_4 = (__pyx_v_ep->me_value != NULL);
[sagelib-8.0.beta7]                           ^
[sagelib-8.0.beta7] /home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:4332:46: error: request for member 'me_value' in something not a structure or union
[sagelib-8.0.beta7]    __pyx_t_4 = (PyWeakref_GetObject(__pyx_v_ep->me_value) != Py_None);
[sagelib-8.0.beta7]                                               ^
[sagelib-8.0.beta7] error: command 'gcc' failed with exit status 1

comment:45 Changed 5 years ago by Nils Bruin

Are you testing the right branch? PyDictEntry doesn't occur in weak_dict.pyx. The only references to it are in dict_del_by_value.pyx and they don't get exported.

comment:46 Changed 5 years ago by Frédéric Chapoton

hmm, indeed. I was trying the vanilla develop branch... sorry for the noise..

comment:47 Changed 5 years ago by Travis Scrimshaw

Status: needs_workpositive_review

comment:48 Changed 5 years ago by Frédéric Chapoton

Indeed, it works with SAGE_PYTHON3=yes, so I agree with the positive review.

comment:49 Changed 5 years ago by Travis Scrimshaw

Ah, sorry, I didn't realize you were (re?)checking it. I saw comment:46 and thought you just forgot to flip it back to positive review.

comment:50 Changed 5 years ago by Volker Braun

Branch: public/2230554d8c2da8ad19e610fe9939b90bc23202c5b5525
Resolution: fixed
Status: positive_reviewclosed

comment:51 Changed 5 years ago by Frédéric Chapoton

Commit: 54d8c2da8ad19e610fe9939b90bc23202c5b5525

could the changes made here possibly explain the issue arising there:

Traceback (most recent call last):
  File "/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/rings/polynomial/polynomial_ring_constructor.py", line 604, in _get_from_cache
    return _cache[key]
  File "sage/misc/weak_dict.pyx", line 699, in sage.misc.weak_dict.WeakValueDictionary.__getitem__ (/home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:3507)
TypeError: unhashable type: 'ComplexIntervalField_class_with_category'

during an experimental python3-build of sage ?

comment:52 in reply to:  51 Changed 5 years ago by Nils Bruin

Replying to chapoton:

could the changes made here possibly explain the issue arising there:

Traceback (most recent call last):
  File "/home/chapoton/sage3/local/lib/python3.6/site-packages/sage/rings/polynomial/polynomial_ring_constructor.py", line 604, in _get_from_cache
    return _cache[key]
  File "sage/misc/weak_dict.pyx", line 699, in sage.misc.weak_dict.WeakValueDictionary.__getitem__ (/home/chapoton/sage3/src/build/cythonized/sage/misc/weak_dict.c:3507)
TypeError: unhashable type: 'ComplexIntervalField_class_with_category'

during an experimental python3-build of sage ?

No.

Also, it's not such a good idea to ask questions on closed tickets. Probably no-one will read them, unless they happen to be looking at the ticket for reference for something else.

Note: See TracTickets for help on using tickets.