BLD: compatibility with Cython 3.0 #4355
Comments
|
The last item without a PR on the "blockers" list is a little hard for me to decipher. It only affects For reference, here's a simple script that can be used for testing compilation python -m pip install -U --pre Cython build
python -m build --no-isolation .update: nevermind I think I figured it out |
|
We are currently compatible with Cython 3.0.0b1, but I note that some tests are showing performance regressions. I will investigate this, and report anything useful upstream. |
|
I can't do this right now, but probably the most effective thing to do would be to run |
|
Taking the worst offender as a starting point ( from time import monotonic
import numpy as np
from yt.testing import fake_random_ds
from yt.visualization.volume_rendering.api import volume_render
tstart = monotonic()
ds = fake_random_ds(8)
dd = ds.sphere(ds.domain_center, ds.domain_width[0] / 2)
im, sc = volume_render(dd, field=("gas", "density"))
tstop = monotonic()
print(f"{(tstop-tstart)*1000:.0f} ms")and still see the perf difference.
|
|
@neutrinoceros Can you post the |
|
Here are the annotations |
|
Well, I'm not sure anymore. Here's the new generated code: +224: cdef void _set_up_triangles(self, np.float64_t[:, :] vertices,
static void __pyx_f_2yt_9utilities_3lib_25bounding_volume_hierarchy_3BVH__set_up_triangles(struct __pyx_obj_2yt_9utilities_3lib_25bounding_volume_hierarchy_BVH *__pyx_v_self, __Pyx_memviewslice __pyx_v_vertices, __Pyx_memviewslice __pyx_v_indices) {
__pyx_t_5numpy_int64_t __pyx_v_offset;
__pyx_t_5numpy_int64_t __pyx_v_tri_index;
__pyx_t_5numpy_int64_t __pyx_v_v0;
__pyx_t_5numpy_int64_t __pyx_v_v1;
__pyx_t_5numpy_int64_t __pyx_v_v2;
struct __pyx_t_2yt_9utilities_3lib_10primitives_Triangle *__pyx_v_tri;
__pyx_t_5numpy_int64_t __pyx_v_i;
__pyx_t_5numpy_int64_t __pyx_v_j;
__pyx_t_5numpy_int64_t __pyx_v_k;
/* … */
/* function exit code */
goto __pyx_L0;
__pyx_L1_error:;
#ifdef WITH_THREAD
__pyx_gilstate_save = __Pyx_PyGILState_Ensure();
#endif
__Pyx_AddTraceback("yt.utilities.lib.bounding_volume_hierarchy.BVH._set_up_triangles", __pyx_clineno, __pyx_lineno, __pyx_filename);
#ifdef WITH_THREAD
__Pyx_PyGILState_Release(__pyx_gilstate_save);
#endif
__pyx_L0:;
}Here's the old generated code: 224: cdef void _set_up_triangles(self, np.float64_t[:, :] vertices,
static void __pyx_f_2yt_9utilities_3lib_25bounding_volume_hierarchy_3BVH__set_up_triangles(struct __pyx_obj_2yt_9utilities_3lib_25bounding_volume_hierarchy_BVH *__pyx_v_self, __Pyx_memviewslice __pyx_v_vertices, __Pyx_memviewslice __pyx_v_indices) {
__pyx_t_5numpy_int64_t __pyx_v_offset;
__pyx_t_5numpy_int64_t __pyx_v_tri_index;
__pyx_t_5numpy_int64_t __pyx_v_v0;
__pyx_t_5numpy_int64_t __pyx_v_v1;
__pyx_t_5numpy_int64_t __pyx_v_v2;
struct __pyx_t_2yt_9utilities_3lib_10primitives_Triangle *__pyx_v_tri;
__pyx_t_5numpy_int64_t __pyx_v_i;
__pyx_t_5numpy_int64_t __pyx_v_j;
__pyx_t_5numpy_int64_t __pyx_v_k;
/* … */
/* function exit code */
}at first glance it looks like it's doing a bunch of GIL setting, etc. But, that should only be called in an error state -- which in principle should not be the dominant cost. So at this point, I'm not entirely sure what the issue is, unless I am misunderstanding and this code gets called a bunch of times when I think it should not. Now, I looked a little bit more, and there is another difference, also related to the GIL. Again, this shouldn't be an issue, though? In the call to __pyx_v_self->get_centroid(__pyx_v_self->primitives, __pyx_v_tri_index, (__pyx_v_self->centroids[__pyx_v_tri_index])); if (unlikely(__Pyx_ErrOccurredWithGIL())) __PYX_ERR(0, 245, __pyx_L1_error)So it's also possible now that we're calling |
|
While I'm far from mastering every relevant details here, I think you might find this paragraph enlightening (maybe ?) |
|
Seems like the right usage of the legacy option to me!
…On Fri, Mar 17, 2023, 10:46 AM Clément Robert ***@***.***> wrote:
While I'm far from mastering every relevant details here, I think you
might find this paragraph enlightening (maybe ?)
https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#exception-values-and-noexcept
—
Reply to this email directly, view it on GitHub
<#4355 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXOYIAQGBXGGAPOAWMJTW4SBMVANCNFSM6AAAAAAVNEHAUE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Looks appropriate indeed. I tried this patch diff --git a/yt/utilities/lib/bounding_volume_hierarchy.pyx b/yt/utilities/lib/bounding_volume_hierarchy.pyx
index a743257..13f2384 100644
--- a/yt/utilities/lib/bounding_volume_hierarchy.pyx
+++ b/yt/utilities/lib/bounding_volume_hierarchy.pyx
@@ -1,3 +1,4 @@
+# cython: legacy_implicit_noexcept=True
# distutils: libraries = STD_LIBS
# distutils: include_dirs = LIB_DIR
# distutils: language = c++but it fails to compile on both versions (Cython 0.29.33 doesn't recognise this option, and Cython 3.0.0b1 crashes in a more exotic way). At this point I think it's worth reporting ? |
|
Nevermind, the reason it didn't compile is that I forgot to also include the directive in the header, resulting in mismatches between declarations and implementations. |
|
This seems related cython/cython#5327 |
|
Looks like the problem was adressed upstream cython/cython#5324 (assuming it's indeed the same issue). |
|
Cython 3.0.0b2 still shows degraded performance (there are no obvious differences with 3.0.0b1). I really need to take a minute to report this properly. |
|
Enabling profiling in Cython helped me determine that the main offender is An uneducated look at the code generated for these calls to For instance, for this line iter[0] = i64clip(iter[0]-1, 0, self.nv[0])this is the old generated code And this is the new code I further note that the code generated for the I tried applying the |
|
I was close to solving it: in fact what's needed is to add the |
|
Haha, we call those |
|
"fp" : functional programming ? |
|
We've had 3 runs of bleeding edge CI with Cython 3.0.0b2 so far and they seem to take even longer than with 3.0.0b1: now tests take as much as 18min (previously 13, ref is 5). The itch is only getting stronger. |
|
|
|
Yes, I'll take a look. |
|
I've escalated this question to Cython's user mailing list, hopefully it'll get some attention |
|
In the end it's again all about sprinkling Next in line is from time import monotonic
from yt.testing import fake_amr_ds
ds = fake_amr_ds(fields=["density"], units=["g/cm**3"])
ds.add_gradient_fields(("gas", "density"))
ad = ds.all_data()
tstart = monotonic()
ret = ad[("gas", "density_gradient_x")]
tstop = monotonic()
print(f"{(tstop-tstart)*1000:.0f} ms")(on my laptop it takes about 2200ms with Cython 0.29 and 3500ms with Cython 3.0.0b2) |
|
This is now also addressed in #4390 |
|
Already identified performance issues are fixed, as far as I can tell. We should be able to close this issue when we do the switch from |
|
#4575 fixes the remaining issues with, and switch requirements to Cython>=3.0,<3.1 |
Cython 3.0 is now in beta (after several years of alpha).
#4043 contains an initial effort to discover and patch incompatibilities, but at the moment it seems to me that this migration is better done incrementally, as only some of the patches will be seamless and uncontroversial.
I'll be using this issue to track progress on this effort.
The most urgent issues are listed in the following. A useful resource for this task is the migration guide.
blockers
restoring performance
performance issues are discussed in the present thread.
deprecation warnings
The 'DEF' statement is deprecated and will be removed in a future Cython version.Since the migration path for some items is a moving target (see cython/cython#5242), I think the minimal effort approach would be to set our requirement to
Cython>=3.0,<3.1at first. According to the current plan for Cython 3.1 and 3.2, we should be able to drop the upper limit completely once we reach compatibility with version 3.2low priority (expected gains from dropping Cython 0.29.x)
This one is easy, as it just requires adding some compiler flags to all
*.pyxfiles, but it requires dropping Cython 0.29.x first.For future reference, here's a simple way to apply the change systematically
Details
with
header.txt# distutils: define_macros=NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSIONand
t.shapply the change as
PY_LIMITED_APIhttps://cython.readthedocs.io/en/latest/src/changes.html?highlight=CYTHON_LIMITED_API#id27The text was updated successfully, but these errors were encountered: