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

Broken scikit-image build with 3.0a3 : redeclaration of ‘__pyx_gilstate_save’ with no linkage #3558

Closed
emmanuelle opened this issue Apr 28, 2020 · 5 comments

Comments

@emmanuelle
Copy link

The scikit-image pre builds is broken since Cython 3.0a3 was released yesterday.

The error is as follows:

skimage/graph/heap.c: In function ‘__pyx_f_7skimage_5graph_4heap_10BinaryHeap__add_or_remove_level’:
skimage/graph/heap.c:3299:20: error: redeclaration of ‘__pyx_gilstate_save’ with no linkage
   PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
                    ^~~~~~~~~~~~~~~~~~~
skimage/graph/heap.c:3296:20: note: previous declaration of ‘__pyx_gilstate_save’ was here
   PyGILState_STATE __pyx_gilstate_save;
                    ^~~~~~~~~~~~~~~~~~~

corresponding to this part of heap.pyx (https://github.com/scikit-image/scikit-image/blob/master/skimage/graph/heap.pyx#L190)

cdef void _add_or_remove_level(self, LEVELS_T add_or_remove) nogil:
        # init indexing ints
        cdef INDEX_T i, i1, i2, n

        # new amount of levels
        cdef LEVELS_T new_levels = self.levels + add_or_remove

        # allocate new arrays
        cdef INDEX_T number = 2**new_levels
        cdef VALUE_T *values
        cdef REFERENCE_T *references
        values = <VALUE_T *>malloc(number * 2 * sizeof(VALUE_T))
        references = <REFERENCE_T *>malloc(number * sizeof(REFERENCE_T))
        if values is NULL or references is NULL:
            free(values)
            free(references)
            with gil:
                raise MemoryError()

When I compare the generated C code with 3.0a1 and 3.0a2, only these part changed
3.0a1

#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
#endif

and
3.0a3

#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save;
#endif
#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
#endif

so indeed __pyx_gilstate_save is defined twice.

Any advice about what is going on will be much appreciated :-).

If it helps, here is a bit more of the C code (for 3.0a3):

static void __pyx_f_7skimage_5graph_4heap_10BinaryHeap__add_or_remove_level(struct __pyx_obj_7skimage_5graph_4heap_BinaryHeap *__pyx_v_self, __pyx_t_7skimage_5graph_4heap_LEVELS_T __pyx_v_add_or_remove) {
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i1;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i2;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_n;
  __pyx_t_7skimage_5graph_4heap_LEVELS_T __pyx_v_new_levels;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_number;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_v_values;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_v_references;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_v_old_values;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_v_old_references;
  __Pyx_RefNannyDeclarations
  int __pyx_t_1;
  int __pyx_t_2;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_3;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_4;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_5;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_t_6;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_t_7;
  #ifdef WITH_THREAD
  PyGILState_STATE __pyx_gilstate_save;
  #endif
  #ifdef WITH_THREAD
  PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
  #endif
  __Pyx_RefNannySetupContext("_add_or_remove_level", 0);
  #ifdef WITH_THREAD
  __Pyx_PyGILState_Release(__pyx_gilstate_save);
  #endif
@scoder scoder added this to the 3.0 milestone Apr 28, 2020
@scoder scoder closed this as completed in 01085d2 Apr 28, 2020
@scoder
Copy link
Contributor

scoder commented Apr 28, 2020

Thanks for the detailed report @emmanuelle. The change in 7d99b0f didn't work in nogil functions/methods that received Python arguments as input. Fix pushed, test added.

@scoder
Copy link
Contributor

scoder commented Apr 28, 2020

Note, BTW, that the method signature prevents exception propagation:
cdef void _add_or_remove_level(self, LEVELS_T add_or_remove) nogil:
Instead, either change the return type from void to something else, e.g. int (with except -1 or similar), or declare the method as except * to make the caller always check for exceptions.

Oh, and the with gil: is no longer required for the raise since 0.29. It's injected automatically when raise is used in a nogil section.

@emmanuelle
Copy link
Author

Thank you very much for the fast and detailed answer (as always ;-)), @scoder! We will improve this function as you suggested.

@da-woods
Copy link
Contributor

Should having a function that can raise an exception but can't return it generate a warning when cythonized? It seems like Cython can identify this (since it generates code for warn-unraisable).

Sub-questions might be: is the identification good enough to avoid too many false positives? are the good reasons why users might want to write code that swallows exceptions like this (and if so is, would there be a good way to mark code like this e.g. except None so it doesn't create a warning)?

@scoder
Copy link
Contributor

scoder commented Apr 29, 2020

Cython generates a warning here, but with warning level 0, i.e. it's normally invisible. Setting Cython.Compiler.Errors.LEVEL = 0 will show it. (I guess there should be a better interface to that, preferably a keyword argument in cythonize()).

oleksandr-pavlyk pushed a commit to oleksandr-pavlyk/cython that referenced this issue Apr 18, 2022
oleksandr-pavlyk pushed a commit to oleksandr-pavlyk/cython that referenced this issue Apr 18, 2022
oleksandr-pavlyk pushed a commit to oleksandr-pavlyk/cython that referenced this issue Apr 18, 2022
…d not just nogil sections.

Closes cython#3558.

Removing tests/run/with_gil_automatic.pyx
scoder pushed a commit that referenced this issue May 3, 2022
)

Closes #4637
See See #3556

* Acquire the GIL in nogil functions only when strictly needed on function exit, e.g. for cleaning up temp variables from with-gil blocks or adding tracebacks.

See #3554

* Make the GIL-avoidance in 7d99b0f actually work in nogil functions and not just nogil sections.

See #3558
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

3 participants