#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: | jdemeyer, 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: |
Description (last modified by )
The structure of PyDictObject
used by weak_dict
changed from Python 2 to Python 3, e.g PyDictEntry
has been removed
REF:
Attachments (1)
Change History (53)
comment:1 Changed 6 years ago by
comment:2 Changed 5 years ago by
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 5 years ago by
- Description modified (diff)
comment:4 Changed 5 years ago by
- Type changed from PLEASE CHANGE to enhancement
comment:5 Changed 5 years ago by
- Cc jdemeyer added
This is one of our cython+python3 compilation problems.
comment:6 Changed 5 years ago by
- Milestone changed from sage-7.6 to sage-8.0
comment:7 Changed 5 years ago by
Could somebody do some action on the problem here, please ?
comment:8 Changed 5 years ago by
This is probably the last cython file that does not compile with python3 !!
comment:9 Changed 5 years ago by
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
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?
comment:11 Changed 5 years ago by
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
comment:12 Changed 5 years ago by
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
- Cc 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
i have no available time this week
comment:15 Changed 5 years ago by
- Branch set to public/22305
- Commit set to 02a2b13df369e26bb463c6bf2429c7873a4f972c
comment:16 Changed 5 years ago by
- Commit changed from 02a2b13df369e26bb463c6bf2429c7873a4f972c to 787975ecdfe248586d6bafcfe407d566096e24bc
Branch pushed to git repo; I updated commit sha1. New commits:
787975e | trac 22305 adding the new weak_dict.pyx
|
comment:17 Changed 5 years ago by
I made a git branch with your latest patch.
EDIT: it was not cythonizing correctly because of a typo. Now corrected in latest commit.
comment:18 Changed 5 years ago by
- Commit changed from 787975ecdfe248586d6bafcfe407d566096e24bc to 8722eb7a8fbbef4b6bdc94f19a7a69ea1e238cbd
Branch pushed to git repo; I updated commit sha1. New commits:
8722eb7 | trac 22305 typo
|
comment:19 follow-up: ↓ 20 Changed 5 years ago by
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
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
@jdemeyer, do you know how to achieve properly this kind of py2/py3 switch in cython code ?
comment:22 Changed 5 years ago by
I'll have a look.
comment:23 Changed 5 years ago by
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
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
My proposal would be:
- Make a separate ticket to deal with changes to
weak_dict.pyx
which are good to do anyway regardless of Python vesion.
- Patch Python 3 to make public whatever is needed for the Python 3 version of
weak_dict.pyx
.
- Finally, use conditional compilation to deal with Python 2/3 differences in
weak_dict.pyx
.
comment:26 Changed 5 years ago by
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
ping ? Jeroen, if you want to make a ticket for your point 1, please do so.
comment:28 Changed 5 years ago by
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
- Commit changed from 8722eb7a8fbbef4b6bdc94f19a7a69ea1e238cbd to 4c73dbec8a910509aa837233f878099e62a379ed
comment:30 Changed 5 years ago by
- Commit changed from 4c73dbec8a910509aa837233f878099e62a379ed to 71b68d22215182fa7700ddb73ecd1bf82fda84f4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
71b68d2 | tract #22305 -- split off del_dictitem_by_exact_value and compile conditionally based on Py2/Py3
|
comment:31 Changed 5 years ago by
- Status changed from new to needs_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
- Commit changed from 71b68d22215182fa7700ddb73ecd1bf82fda84f4 to 54d8c2da8ad19e610fe9939b90bc23202c5b5525
Branch pushed to git repo; I updated commit sha1. New commits:
54d8c2d | Correction: Py3 does provide PyDict_GetItemWithError but Cython doesn't provide it yet
|
comment:33 Changed 5 years ago by
seems to be ok on python2 sage, see patchbot report
I will try with SAGE_PYTHON3=yes
comment:34 Changed 5 years ago by
hum, it seems that latest update to numpy broke the SAGE_PYTHON3 build..
comment:35 Changed 5 years ago by
See #22582 for a numpy upgrade which should fix the python 3 build.
comment:36 Changed 5 years ago by
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
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 follow-up: ↓ 39 Changed 5 years ago by
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
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
I have created #22942
comment:41 Changed 5 years ago by
- Dependencies set to #22942
comment:42 Changed 5 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, green bot, I am setting to positive.
comment:43 Changed 5 years ago by
- Status changed from positive_review to needs_work
hmm, does not seem to compile with SAGE_PYTHON3=yes and python3.6.1
comment:44 Changed 5 years ago by
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
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
hmm, indeed. I was trying the vanilla develop branch... sorry for the noise..
comment:47 Changed 5 years ago by
- Status changed from needs_work to positive_review
comment:48 Changed 5 years ago by
Indeed, it works with SAGE_PYTHON3=yes, so I agree with the positive review.
comment:49 Changed 5 years ago by
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
- Branch changed from public/22305 to 54d8c2da8ad19e610fe9939b90bc23202c5b5525
- Resolution set to fixed
- Status changed from positive_review to closed
comment:51 follow-up: ↓ 52 Changed 5 years ago by
- Commit 54d8c2da8ad19e610fe9939b90bc23202c5b5525 deleted
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
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.
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.
Also, perhaps we should always force the WeakValueDict? to be "combined" (the old-style layout), because split tables don't allow deletion, and triggering reshaping of the dictionary in a weakref callback is probably not a good idea.