Closed alexsavulescu closed 3 weeks ago
@sgamiye can you look at the rxd stuff? Ideally, try to make it so that it builds in both 3.0 and before. (e.g. replace xrange
with range
, etc)
Just for the record, using Cython3 result into error like does not support 3d mode (no "volumes3d" attr)
:
simple_model = ((<TopLevelHocInterpreter>, <module 'neuron.rxd' from '/home/runner/work/nrn/nrn/build/lib/python/neuron/rxd/__init__....05550>, dx=0.25, name='cyt')], name=None, charge=0, value=0), Rate(k, (k)*(-0.1), regions=[cyt], membrane_flux=False)))
def test_reinit_3d(simple_model):
"""Test rxd.re_init updates node values from NEURON values in 3D"""
neuron_instance, model = simple_model
h, rxd, data, save_path = neuron_instance
dend, cyt, k, paramA, paramB, decay = model
rxd.set_solve_type(dimension=3)
# check changing the units after initialization
> h.finitialize(-65)
E RuntimeError: hocobj_call error
test/rxd/test_reinit.py:55: RuntimeError
----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
File "/home/runner/work/nrn/nrn/build/lib/python/neuron/rxd/rxd.py", line 1855, in _init
initializer._do_init()
File "/home/runner/work/nrn/nrn/build/lib/python/neuron/rxd/initializer.py", line 32, in _do_init
obj._do_init()
File "/home/runner/work/nrn/nrn/build/lib/python/neuron/rxd/region.py", line 680, in _do_init
raise RxDException(
neuron.rxd.rxdException.RxDException: selected geometry (<neuron.rxd.geometry.RxDGeometry object at 0x7ffb32705550>) does not support 3d mode (no "volumes3d" attr)
@ramcdougal : I was reading NumPy 2.0-specific advice from docs here. It doesn't say specifically about Cython 3 aspects but as we are now talking about ABI compatibility aspects, I believe it would be good idea to have latest Cython (v3) working. This way, we are sure that we are up to date with the toolchain and we don't see corner cases.
Someone would have time to tackle this? (we lack knowledge on rxd side of testing and hence asking)
I just wanted to add the necessary information from past debugging so that this ticket is up to date and it would be easy to debug further efficiently:
share/lib/python/neuron/rxd/geometry3d/
has *.pyx
file and our top-level setup.py
uses Cython.Distutils
to convert Cython to C++ here:$ ls share/lib/python/neuron/rxd/geometry3d/
FullJoinMorph.py ctng.pyx simplevolume_helper.py surfaces.pyx
GeneralizedVoxelization.py graphicsPrimitives.cpp surface.py triangularMesh.py
__init__.py graphicsPrimitives.pyx surface_a.py voxelize.py
ctng.cpp scalarField.py surfaces.cpp voxelize2.py
So if you are building with different Cython versions, make sure to build in different source/clone directories otherwise cython files are overwritten.
This ticket mentions MUSIC but I am not sure there is an issue with cython part of MUSIC. Most likely not. But if there is an issue, that could be tackled last. Don't need to worry about that part to begin with.
To reproduce the issue with Cython 3, we should build neuron with standard instructions. See these quick changes from pramodk/cython3-debug
branch that will help to build & debug the issue.
With that changeset, here are commands for completeness:
python3 -m venv env-cython3
. env-cython3/bin/activate
pip3 install "numpy<2" "cython>3" plotly matplotlib pytest ipywidgets plotly # last one was needed on my ubuntu
git clone git@github.com:neuronsimulator/nrn.git nrn-cython3 -b pramodk/cython3-debug #using branch with the changeset mentioned
mkdir nrn-cython3/build && cd nrn-cython3/build
cmake .. \
-DNRN_ENABLE_INTERVIEWS=OFF \
-DNRN_ENABLE_MPI=OFF \
-DNRN_ENABLE_RX3D=ON \
-DNRN_ENABLE_TESTS=ON \
-DCMAKE_INSTALL_PREFIX=$(pwd)/install
make -j6
$ ctest -R rxd
Test project /home/kumbhar/tmp/tmp/nrn-cython3/build
Start 82: rxdmod_tests::rxd_tests
1/1 Test #82: rxdmod_tests::rxd_tests ..........***Failed 1.95 sec
0% tests passed, 1 tests failed out of 1
Note: if the test is passed with Cython 3, verify if correct Cython3 is picked up by CMake!
The error that we have mentioned in earlier comment is the following:
82: neuron.rxd.rxdException.RxDException: selected geometry (<neuron.rxd.geometry.RxDGeometry object at 0x7fd89f1af130>) does not support 3d mode (no "volumes3d" attr)
We get this error because here we try to import geometry3d
module. If fails we are simply saying has_geometry3d = False
and hence the errors like the above.
With the traceback.print_exc()
that I have added in the changeset, we can see the actual error:
$ ctest -R rxd -V
...
82: ImportError: /home/kumbhar/tmp/tmp/nrn-cython3/build/lib/python/neuron/rxd/geometry3d/surfaces.cpython-310-x86_64-linux-gnu.so:
undefined symbol: _Z10llgramareaPdS_S_
82: NEURON: Python Callback failed [hoccommand_exec]:
...
i.e. when module is being imported, the shared library couldn't find llgramarea
symbol. A few points worth mentioning:
llgramarea
comes from? It's from src/nrnpython/rxd_llgramarea.cpp$ grep -r llgramarea share/lib/python/neuron/rxd/geometry3d/*.py*
share/lib/python/neuron/rxd/geometry3d/surface_a.py: llgramarea = nrn_dll_sym("llgramarea")
share/lib/python/neuron/rxd/geometry3d/surface_a.py: llgramarea.argtypes = [POINTER(c_double), POINTER(c_double), POINTER(c_double)]
share/lib/python/neuron/rxd/geometry3d/surface_a.py: llgramarea.restype = c_double
share/lib/python/neuron/rxd/geometry3d/surface_a.py: area += llgramarea(pt0_ptr, pt1_ptr, pt2_ptr) / 2.0
share/lib/python/neuron/rxd/geometry3d/surfaces.pyx:cdef extern double llgramarea(double* p0, double* p1, double* p2)
share/lib/python/neuron/rxd/geometry3d/surfaces.pyx: local_area = llgramarea(&triangles[i], &triangles[3 + i], &triangles[6 + i])
xd_llgramarea.o
has correct unmangled name:$ nm src/nrnpython/rxd_llgramarea.o | grep llgramarea
0000000000000000 T llgramarea
But the surfaces.o
from Cython generated CPP file uses mangled name:
$ nm ./share/lib/python/neuron/rxd/geometry3d/surfaces.o | grep llgramarea
U _Z10llgramareaPdS_S_
If look into my build directory with "Cython<3" then I see:
$ nm ./share/lib/python/neuron/rxd/geometry3d/surfaces.o | grep llgramarea
U llgramarea
i.e. as expected, it's using an unmangled name (expected due to "extern C").
llgramarea()
end up using a mangled name. Hopefully there is something minor thing missing / change required consideing the nature of the issue.@ceciliaromaro - if you haven't started on this issue and if you are busy with other things, we have asked @kbvw to help with this issue.
Note: For the Numpy 2.0 part mentioned in the previous comment, that's a separate issue and should be tackled/tested once we have Cython 3 part working. For the reference, this NumPy 2.0-specific advice was referred in our Slack discussion.
I think @kbvw locally has a fix for this issue.
RXD seems fine with #3028. Many thanks to @1uc for the help.
Regarding MUSIC, there is another guard against Cython 3 in the CMake, which that PR removes, but then I do not see any problems in the CI. What was the reason MUSIC was mentioned here originally?
What was the reason MUSIC was mentioned here originally?
The fix you made also affects MUSIC and could easily also fix the issues there. I only see the original post mention that MUSIC might be affected. Which I interpret as the reporter noticed that MUSIC uses Cython but didn't check if there was a problem. I don't remember getting past the: "crap there's an issue, let's pin" stage until now.
Later Pramod writes:
This ticket mentions MUSIC but I am not sure there is an issue with cython part of MUSIC. Most likely not. [...]
We are currently pinning
Cython < 3
following #2433 (see also #2436). More work required in order to support Cython3.Areas of concern:
msys2 mingw64-cython
version vspip install cython
forofficial Python installations
)