cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
16 stars 21 forks source link

Python docstrings tests: don't test for CellmlElementType #1245

Open agarny opened 1 month ago

agarny commented 1 month ago

I am very confused since it all seems to be working fine on our CI machines, but whenever I test the Python docstrings (using ctest -V -R python_test_docstrings), I get the following on my MacBook Pro using Python 3.12.4:

Test command: /opt/homebrew/Frameworks/Python.framework/Versions/3.12/bin/python3.12 "test_docstrings.py"
Working Directory: /Users/Alan/libCellML/build/tests/bindings/python
Environment variables:
 PYTHONPATH=/Users/Alan/libCellML/build/src/bindings/python
Test timeout computed to be: 10000000
E
======================================================================
ERROR: test_docstrings (__main__.DocstringTestCase.test_docstrings)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Alan/Work/Programming/libCellML/build/tests/bindings/python/test_docstrings.py", line 31, in test_docstrings
    raise Exception(
Exception: Missing (1) docstrings, for: libcellml.CellmlElementType

----------------------------------------------------------------------
Ran 1 test in 0.175s

FAILED (errors=1)
1/1 Test #python_test_docstrings ...........***Failed    0.22 sec

In other words, I get told that there is no docstring for libcellml.CellmlElementType and indeed I can't see one anywhere. However, if I add a docstring for libcellml::CellmlElementType in src/bindings/interface/enums.i, e.g.

--- a/src/bindings/interface/enums.i
+++ b/src/bindings/interface/enums.i
@@ -4,6 +4,9 @@

 %include <std_string.i>

+%feature("docstring") libcellml::CellmlElementType
+"Get the :class:`CellmlElementType` of a CellML element.";
+
 %feature("docstring") libcellml::cellmlElementTypeAsString
 "Convert a CellmlElementType enumeration value into its string form.";

then I still get that error message. @hsorby, any idea of what is going on here?

The only way for me to get rid of that error message is to NOT test whether there is a docstring for libcellml::CellmlElementType, i.e.

--- a/tests/bindings/python/test_docstrings.py
+++ b/tests/bindings/python/test_docstrings.py
@@ -13,7 +13,7 @@ class DocstringTestCase(unittest.TestCase):
         # Scan for missing or empty docstrings
         def scan(root, missing, prefix=''):
             prefix += root.__name__
-            if not root.__doc__:
+            if not root.__doc__ and root.__name__ != 'CellmlElementType':
                 missing.append(prefix)
             prefix += '.'
             # Scan children, using dict instead of dir to avoid inherited
nickerso commented 1 month ago

The CI machines seem to be using Python 3.7, 3.8, or 3.9 - is this a 3.12 specific issue?

agarny commented 1 month ago

Yup:

hsorby commented 1 month ago

As a solution here, I would like us to switch to using SWIG to take the Doxygen comments from the C++ and applying it to the Python bindings. This does have some disadvantages but I think the Doxygen documentation is more informative than the current Python bindings documentation. I haven't tested this idea with Python 3.11, or 3.12 to see if it would resolve this particular issue though.

luciansmith commented 1 month ago

I can say from my experience that SWIG is great at what it does, but it also means that you need separate binaries for every version of Python on every platform, which means at least 12 if not 16 binaries for every release. It's certainly doable, but but it is a little much. If you can manage to put something together with ctypes, you'd only need one binary per platform, which would work for all versions of Python.

agarny commented 1 month ago

ctypes

That's all good (although we are still facing the same issue: lack of resources to do nice-to-have things), but what about now? What should we do about this issue?

I can say from my experience that SWIG is great at what it does, but it also means that you need separate binaries for every version of Python on every platform, which means at least 12 if not 16 binaries for every release. It's certainly doable, but but it is a little much. If you can manage to put something together with ctypes, you'd only need one binary per platform, which would work for all versions of Python.

I have never used ctypes, but when it comes to building Python packages, we use cibuildwheel, so it's relatively simple, although I do appreciate your point.

When it comes to SWIG, I do find it a bit of an overkill for libCellML since we "only" provide Python and JavaScript bindings. So, we only use SWIG for our Python bindings (JavaScript bindings are generated using Emscripten). Personally, I wish we would be using pybind11 (or even nanobind).