search

bjodah / chempy

⚗ A package useful for chemistry written in Python
BSD 2-Clause "Simplified" License
548 stars 79 forks source link

pycvodes/pykinsol failing to compile with python 3.9.1 #191

Closed jeremyagray closed 3 years ago

jeremyagray commented 3 years ago

I just upgraded my debian bullseye system to current, which pulled in python 3.9.1 and subsequently triggered a bunch of warnings like:

    In file included from pycvodes/_cvodes.cpp:668:
    external/anyode/include/anyode/anyode_numpy.hpp:205:61: warning: ‘PyObject* PyEval_CallObjectWithKeywords(PyObject*, PyObject*, PyObject*)’ is deprecated [-Wdeprecated-declarations]
      205 |         PyObject * py_result = PyEval_CallObjectWithKeywords(this->py_jac, py_arglist, this->py_kwargs);

in pycvodes/anyode and pykinsol during compilation.

Python references: here and here.

This is just informational. Without further reading, I don't really now how serious this is so this may be fixable by ignoring the deprecation warning.

bjodah commented 3 years ago

Thanks! Yes, one should fix deprecation warnings so that we don't risk having non-compiling versions on e.g. python 3.10. Hopefully these two new releases are free of calls to those functions:

jeremyagray commented 3 years ago

Those versions killed all the deprecations and warnings on both packages. I had made similar changes to pycvodes locally but somehow got a dirty directory and couldn't finish the build; I think I forgot to recursively update anyode.

I still have the same compilation problems with extern "C" versus extern in anyode_blas_lapack.hpp however. I am now installing pycvodes from a local repo with the anyode url in .gitmodules pointed to a local repo with an altered anyode_blas_lapack.hpp. In sundials (sundials_lapack.h), they define the linkage of all the functions they declare as extern "C" for C++. If I understand correctly, the linkage (extern "C") for a function is defined only once whereas you can use an external declaration (extern) where ever necessary. I mention this because now that I have pycvodes installed again, I am still unable to run the chempy tests requiring it and pycvodes is the only package that had a special installation. As soon as I can clear this hurdle, I hope to be able to document all the requirements and begin working on packaging.

On Wed, Dec 30, 2020 at 4:15 PM Bjorn notifications@github.com wrote:

Thanks! Yes, one should fix deprecation warnings so that we don't risk having non-compiling versions on e.g. python 3.10. Hopefully these two new releases are free of calls to those functions:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/issues/191#issuecomment-752774719, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS75NJBQSKEGCF75O33SXORA5ANCNFSM4VDNHYIA .

-- Jeremy A. Gray

bjodah commented 3 years ago

That sounds great. The trouble with extern is of course worrisome. And yes, I don't think we should re-declare them. Perhaps adding some preprocessor guard in anyode_blas_lapack.hpp based on a macro defined by sundials, and make sure sundials' header is included before anyode in pycvodes would do the trick?

It would be nice if we could reproduce the error from the current implementation in the CI run (.drone.yml) of pycvodes, and then apply a proper fix. But as you know, I wasn't able to do so earlier.

jeremyagray commented 3 years ago

I think this issue can be closed, since my build problems with pycvodes is a separate problem. At this point, I believe I will restart with a set of new, clean environments and see if I can find and reconcile any differences between my environment and the one in .drone.yml in chempy and pycvodes. Now, I am also getting test failure in test_get_native__a_substance_no_composition (solve0 and solve1 tests) in chempy with a ValueError. I think both of those use solvers from gsl, but I don't know if that is the cause.

Regardless, I think it's clear there is something wrong in my build environment, which I may have caused. I hope to find and document it soon.

On Sat, Jan 2, 2021 at 5:50 AM Bjorn notifications@github.com wrote:

That sounds great. The trouble with extern is of course worrisome. And yes, I don't think we should re-declare them. Perhaps adding some preprocessor guard in anyode_blas_lapack.hpp based on a macro defined by sundials, and make sure sundials' header is included before anyode in pycvodes would do the trick?

It would be nice if we could reproduce the error from the current implementation in the CI run (.drone.yml) of pycvodes, and then apply a proper fix. But as you know, I wasn't able to do so earlier.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bjodah/chempy/issues/191#issuecomment-753464438, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOQCHS5A3S2JP2F4NNTQLATSX4CB7ANCNFSM4VDNHYIA .

-- Jeremy A. Gray