-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Python bindings #197
Add Python bindings #197
Conversation
We would like to rely on the fact that the [1] https://libcpuid.sourceforge.net/doxy/group__libcpuid.html#ga1b0131f6d12aca2d002175abcd9a1d20 |
71b2bf0
to
a29cf2d
Compare
I converted the PR to a draft because I'm still working on the tests, but feedback would be appreciated nonetheless: especially whether the binding could actually be accepted into this repository once polished. |
Ping @TheTumultuousUnicornOfDarkness since there is no feedback yet: is it realistic to expect that this addition will eventually be reviewed and merged? An alternative is maintaining the binding in a separate repository, but having it incorporated in libcpuid would make the packaging process much simpler in Fedora/RHEL (where the binding will be used by TuneD). |
The PR is still marked as a draft. I think the merge/review doesn't hurry, but it would be great to have a feedback from the libcpuid upstream maintainer whether we are on a good path with the PR and whether it's realistic to expect that the binding will be accepted into libcpuid. |
@zacikpa I cannot truly review the python-side of the work, but I will get someone I trust involved so we can provide more comprehensive feedback. Bearing that in mind,
All in all, this is great work and I'd love seeing it in the main project. |
I see you edited your message. anrieff is the project leader, I am just a contributor. Personally, I am not against this feature, but I do not have experience in Python bindings so I do not think I am legitimate to review this PR. |
ARM support? This would be really great. It's a real mess with ARM :) |
1d72972
to
ca0f640
Compare
1a2bcc1
to
da0de29
Compare
@anrieff @TheTumultuousUnicornOfDarkness Thanks to both of you for the positive feedback. I've just converted the bindings from
As per your feedback, I updated the function to a property called import libcpuid
from libcpuid.enums import CPUFeature
cpuid = libcpuid.cpu_identify()
print(CPUFeature.FPU in cpuid.flags)
I'm now working on extending the binding further, so I'll surely include it. However, I believe that the bindings do not have to be complete to be reviewed and merged. What's your opinion on that? I would like to make the PR ready to review after I finally add those tests 😄. |
8bd1c35
to
820b903
Compare
The bindings now cover the entire functionality of libcpuid and there are some simple sanity tests, so I'm converting this PR to review-ready. I also did some refactoring of the interface, see the live docs for reference. The PR includes two commits that slightly change the C library itself. Commit c3eeb48 may be breaking, so I'm open to discussing whether it's appropriate (if not, I can implement a workaround). For now, manual testing can be done by installing the libcpuid C library at 9eb3cf3 normally (make sure that python3 -m pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ libcpuid This installs the Python package from the TestPyPI repository. |
I just realized I did no respond to this comment:
Yes, such tools exist, but I believe the resulting API would be very C-like. For example, you would have to manually free instances of |
About c3eeb48: About 9eb3cf3: @anrieff, I let you review/merge this PR if you agree. |
820b903
to
bac0cb1
Compare
This makes the option names of cpu_feature_level_t consistent and it also makes it easier to list options of the cpu_feature_t enum just by their prefix (because it now doesn't collide with the prefix of cpu_feature_level_t options).
The bindings are implemented via python-cffi and cover all current functionality of the library. They do not provide a 1-to-1 mapping of the functionality, but a more "Pythonic" API instead. When new functionality is added to the libcpuid library in the future, the changes must be manually added to the bindings in order to appear in the Python interface. However, if only a C enum is extended, the changes will be automatically reflected in the Python interface.
Additionally, setup the documentation for deployment to Read The Docs.
And include them in the CI together with pylint and formatting checks.
bac0cb1
to
d63a177
Compare
Thank you for your feedback, @TheTumultuousUnicornOfDarkness. Regarding 9eb3cf3: I also wasn't content with the change, so I reverted it and replaced it with another solution (though quite hackish). Essentially, the reason for the change was that CFFI cannot parse |
Nice! The About the documentation, how does it work, is the documentation automatically updated? It looks good to me. I would like to see this PR merged before we release the next version (i.e. v0.7.0). @anrieff is probably on holiday, maybe I can merge this PR myself. |
Yes, the documentation is automatically built with each commit, currently from my fork. If this gets merged, I would take it down to free the domain and either you or @anrieff could set it up on the main repo. The easiest setup is essentially just logging in to https://readthedocs.org via a GitHub account, plus a couple clicks to import the repo. |
9915832
into
anrieff:master
Thanks for merging, @TheTumultuousUnicornOfDarkness. I just released the domain libcpuid.readthedocs.io, so it should be possible to set up the documentation at the domain for the main repo. |
This PR adds Python bindings in the form of a complete Python package.
Features
The bindings are implemented via
ctypes
and currently include a subset oflibcpuid
functionality (mainlycpu_identify
andget_cpu_list
). I did not intend to provide a one-to-one API; instead, I aimed for a more Pythonic one.Documentation
The documentation for the package can be build using Sphinx. Currently, it is live at https://libcpuid.readthedocs.io/en/latest/index.html (built from my fork). Ideally, the live docs could be built from this repository (once a repo is registered and linked to a readthedocs account, the build process of live docs requires no maintenance).
PyPi
I intend to publish the package also at PyPi. Currently, it is published at TestPyPi (https://test.pypi.org/project/libcpuid) and can be installed using
pip install -i https://test.pypi.org/simple/ libcpuid
.Why?
As developers of TuneD (https://github.com/redhat-performance/tuned), we would like to use
libcpuid
functionality from Python to be able to restrict certain system tunings to e.g., systems with specific CPU models or their successors. In other words, we want to utilizecpu_identify
and also the fact thatget_cpu_list
claims to return the list of CPU models in the order in which they were introduced.