Revive the xNVMe Python language bindings #78

safl · 2022-04-25T19:28:55Z

xNVMe previously had Python bindings, however, these where manually written wrappers which were not properly tested, thus they became stale and were removed from the repository.

However, recently when providing examples of how to dynamically load xNVMe from C and Python, then it seemed worth-while to explore just how much is possible via ctypes. With a couple of non-intrusive changes to the public C API (adding a couple of includes), then it is possible to auto-generate a complete ctypes-wrapping for the C API. With the essentials auto-generated, then using ctypes is a breeze.

With this PR a full xNVMe C API mapping is available along with automatic dynamic loading. This module can serve for those wanting to experiment with the API from the comforts of Python. Additionally, as a reference for adding Cython bindings as well, for those going beyond poking at the API.

Add testcases and testplan for CIJOE invoking and verifying the library
Add testing to CI
Add description of the Python module in the docs

Have a look at the toolbox/py_xnvme_pkg/README.rst for more information.

EDIT: Have a look at the docs in docs/python/index.rst for more information.

.github/workflows/verify.yml

toolbox/py_xnvme_pkg/setup.py

toolbox/py_xnvme_pkg/Makefile

Baekalfen · 2022-05-04T09:30:07Z

toolbox/py_xnvme_pkg/Makefile

+.PHONY: generate-ctypes
+generate-ctypes:
+	@echo "## py: gen"
+	@clang2py ${XNVME_CAPI_HEADERS} > xnvme/ctypes_mapping.py


I had to specify additional arguments for macOS: @clang2py --clang-args="-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk/usr/include/ -I../../include/" ${XNVME_CAPI_HEADERS} > xnvme/ctypes_mapping.py

Hmm, curious. This is probably something to explore further. To get this moving along then the build-testing is restricted to a single platform, the package generated and then tested on all the usual suspects.

toolbox/py_xnvme_pkg/xnvme/ctypes_mapping.py

toolbox/py_xnvme_pkg/xnvme/library_loader.py

.github/workflows/verify.yml

docs/python/index.rst

Baekalfen · 2022-05-30T13:06:38Z

python/bin/xpy_dev_open

+
+import xnvme.ctypes_mapping as capi
+
+capi.guard_unloadable()


I think it's better to move this to the library.

How about this would go on the list of things to refine in docs/python/index.rst? Have done so now.

Baekalfen · 2022-05-30T13:08:23Z

python/bin/xpy_enumerate

+
+import xnvme.ctypes_mapping as capi
+
+capi.guard_unloadable()


How about this would go on the list of things to refine in docs/python/index.rst? Have done so now.

Baekalfen · 2022-05-30T13:09:54Z

python/setup.py

+    package_dir={"": "./"},
+    packages=setuptools.find_packages(where="./"),
+    python_requires=">=3.6",
+    install_requires="pytest",


Install wouldn't require pytest. Either use tests_require or now that you have a python/pyproject.toml, put it there instead.

I have gone with the approach of installing the testcases, this allows the ease-of-use of running the pytest via the package-discovery. This is why pytest is added as a runtime requirement.

I still think we should not have it here, and put it in either tests_require or pyproject.toml, as it's not part of normal runtime usage.

Also, isn't it supposed to be a list? ["pytest"]?

By having it as install_requires then pytest is installed along with the package.
By doing so and by adding the library-tests with the package, then it becomes incredibly easy to verify that the bindings are working as intended by invoking:

python3 -m pytest --pyargs xnvme

I don't see any reason not to do this.

The reason not to do it, would be because it's not a runtime dependency. I think it would be exceedingly rare somebody else is running our test suite. And as soon as we merge my Cython PR, the tests would no longer succeed without proper configuration.

But why not put it under the test requirements, if it's for testing?

Adding to that, this is why python3 -m pytest --pyargs xnvme is added as a bold note in the docs right after installation.

Also, this does not dictate that all tests are installed with the package, just the ones useful for the use-case described above.

How about a different approach? Instead of implementing these as a pytest, why not just a simple function?

def self_test(): # Self-test for the dynamically loaded library # The 'capi' is loaded upon module access, this checks that it succeeded assert capi.is_loaded, "C API isn't loaded" # This verifies that the loaded library actually is the xNVMe library, by checking for # the existance of a xNVMe specific funtion. assert capi.xnvme_enumerate, "xnvme_enumerate is not present in library. Library isn't loaded correctly" # Verifies that at at least one function is callable assert capi.xnvme_libconf_pr(capi.XNVME_PR_DEF), "Failed to call function 'xnvme_libconf_pr in xNVMe. Library isn't loaded correctly" print('xNVMe ctypes bindings loaded correctly!')

Then in the docs: python3 -c 'import xnvme.ctypes_bindings as xnvme; xnvme.self_test()'

That is also an approach, the down-side is that then the tests need to added in multiple places and not behaving as nicely as pytest.

I do like the messages on the asserts, so I have added that to the tests.

I don't see any reason not to use pytest for this, the only counter-argument is that it is "unusual" but it is no more unusual than pytest describing the use-case in their documentation and providing test-collection for it. So, its a well-documented and supported feature of pytest.

Should the addition of pytest cause us grief, then it is no worse than we can remove it and replace it with something like the approach above. However, for now I see no reason to, it only makes things simpler/easier to use.

the only counter-argument is that it is "unusual"

The other argument is that this solution is short-lived. As soon as we merge my Cython-to-Python bindings, it won't work anymore.

Should the addition of pytest cause us grief, then it is no worse than we can remove it and replace it with something like the approach above.

If you want to, then keep it and let's see.

python/requirements.txt

python/xnvme/library_loader.py

safl · 2022-05-30T13:40:12Z

@Baekalfen I have addressed your feedback, some with changes, some with clarification, and two things post-poned as "nice-to-have" / added to docs/python/index.rst for future fixing.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

safl · 2022-05-31T10:33:16Z

@Baekalfen found some of your comments on the commits. Addressed those as well and pushed. Thanks for all the input! Your review improved this PR in a lot of ways:

Reduced the build-python from ~50 seconds to ~30 seconds
Removed the generator-requirement needing the xNVMe libraries installed on the system
Removed generated bindings from the repos
Improved the Makefile helper, it now behaves idiomatically
Several fixes on the docs and additions to todo

Thank you!

python/bin/xpy_dev_open

The package requires Python 3.6+, attempting to not go beyond 3.6+ due to lack of availability on a range of Linux distributions. * Makefile: misc. utilities for working with the Python package * The Python package itself - 'python/xnvme' home of the Python package - 'python/xnvme/library_loader.py', provides helpers used for autoloading the xNVMe shared library - 'python/xnvme/ctypes_header.py', autogenerated ctypes encapsulation of the xNVMe C API * Examples - Provided as executables and bundled with the Python package - Available in: 'python/bin/' - Serves as stand-alone usage-examples * Tests - Provided with the Python xnvme package - Available in 'python/xnvme/tests' - Runnable with: 'python3 -m pytest --pyargs xnvme' - Requires pytest * git-ignore: - Added the generated headers, don't want them as part of the repos * pcf: exclude the py-ctypes-mapping from flake8 The remaining files: LICENSE/README.rst/setup.py/pyproject.toml, as the "usual" suspects for a Python package. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

With the revival of the Python language bindings, documentation is needed to describe what they are, how to use them, and the state they are in. This extension to the documentation does so. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

This adds a workflow-job named 'build-python' building the Python language bindings and emitting an installable Python package as a build-artifact named 'python-pkg' / xnvme-py-MAJOR-MINOR-PATCH.tar.gz. The workflow-job, runs before 'build-linux' and 'build-macos' as these depend on it to consume the 'python-pkg' for testing. The downside is that the workflow-pipeline is delayed the time it takes to process the job which is ~30 seconds, arguable this time is reclaimed for the total pipeline to finish since the 'build-python' step runs Alpine and the various tools needed are deployed there much faster than on the other Linux distributions and Linux. A actual shortcoming is that the build of the Python package is only tested in a single environment. However, the main motivation is that the CI emits a single package which is then tested on the various targets, this package can then be promoted for release. Additionally, the package will be tested based on only on the runtime-dependencies defined in (setup.py) and verified that it runs without all the packages defined in requirements.txt. Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

safl · 2022-05-31T13:01:28Z

Thanks @Baekalfen !

safl marked this pull request as draft April 25, 2022 19:29

safl force-pushed the python/ctypes branch 9 times, most recently from fb36e0f to 0e37cce Compare April 27, 2022 09:24

safl changed the title ~~Python/ctypes~~ Revive the xNVMe Python language bindings Apr 27, 2022

safl marked this pull request as ready for review April 27, 2022 09:52

safl force-pushed the python/ctypes branch 2 times, most recently from c857923 to 301d8c0 Compare April 27, 2022 11:49

safl added the pr-ready-for-test PR is ready for functional tests label Apr 27, 2022

safl force-pushed the python/ctypes branch from 301d8c0 to ae64c9d Compare April 27, 2022 13:44

safl removed the pr-ready-for-test PR is ready for functional tests label Apr 27, 2022

safl force-pushed the python/ctypes branch 4 times, most recently from f1761b5 to f5b03f6 Compare April 28, 2022 07:12

Baekalfen reviewed May 4, 2022

View reviewed changes

safl mentioned this pull request May 17, 2022

Changes for v0.5.0 #89

Closed

27 tasks

safl force-pushed the next branch 3 times, most recently from 436c265 to 468ec55 Compare May 18, 2022 18:17

safl force-pushed the python/ctypes branch 4 times, most recently from e8f5baf to 6329319 Compare May 26, 2022 22:11

safl force-pushed the python/ctypes branch 9 times, most recently from 50c5024 to cf86481 Compare May 30, 2022 12:57

Baekalfen reviewed May 30, 2022

View reviewed changes

safl force-pushed the python/ctypes branch from cf86481 to 992f56f Compare May 30, 2022 13:38

safl force-pushed the python/ctypes branch from 992f56f to 285dcbb Compare May 30, 2022 22:23

safl added 2 commits May 31, 2022 09:42

ci: add Python version introspection and platform info

c16a01d

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

ci: add missing pkg-config info

963d90f

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>

safl force-pushed the python/ctypes branch 4 times, most recently from 388c25c to 1010dad Compare May 31, 2022 09:30

Baekalfen reviewed May 31, 2022

View reviewed changes

python/bin/xpy_dev_open Outdated Show resolved Hide resolved

safl force-pushed the python/ctypes branch from 1010dad to 51c538d Compare May 31, 2022 12:28

safl added 3 commits May 31, 2022 14:38

safl force-pushed the python/ctypes branch from 51c538d to 45ec113 Compare May 31, 2022 12:39

Baekalfen approved these changes May 31, 2022

View reviewed changes

safl merged commit 48db24a into xnvme:next May 31, 2022

safl deleted the python/ctypes branch October 20, 2022 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revive the xNVMe Python language bindings #78

Revive the xNVMe Python language bindings #78

safl commented Apr 25, 2022 •

edited

Loading

Baekalfen May 4, 2022

safl May 29, 2022

Baekalfen May 30, 2022

safl May 30, 2022

Baekalfen May 30, 2022

safl May 30, 2022 •

edited

Loading

Baekalfen May 30, 2022

safl May 30, 2022

Baekalfen May 31, 2022

safl May 31, 2022

Baekalfen May 31, 2022

safl May 31, 2022

safl May 31, 2022

Baekalfen May 31, 2022

safl May 31, 2022

Baekalfen May 31, 2022 •

edited

Loading

safl commented May 30, 2022 •

edited

Loading

safl commented May 31, 2022

safl commented May 31, 2022


		import xnvme.ctypes_mapping as capi

		capi.guard_unloadable()


		import xnvme.ctypes_mapping as capi

		capi.guard_unloadable()

Revive the xNVMe Python language bindings #78

Revive the xNVMe Python language bindings #78

Conversation

safl commented Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safl May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baekalfen May 31, 2022 • edited Loading

Choose a reason for hiding this comment

safl commented May 30, 2022 • edited Loading

safl commented May 31, 2022

safl commented May 31, 2022

safl commented Apr 25, 2022 •

edited

Loading

safl May 30, 2022 •

edited

Loading

Baekalfen May 31, 2022 •

edited

Loading

safl commented May 30, 2022 •

edited

Loading