-
Notifications
You must be signed in to change notification settings - Fork 67
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
Adds Python bindings to Caliper with pybind11 #573
Conversation
Great work, thanks @ilumsden! In addition to the documentation it would be great to add an example like the c/cxx/fortran-example apps in the examples/apps directory for the Python annotations. A unit test would also be great. The Caliper unit tests are actually run through Python already so it shouldn't be too difficult to add; check out the tests/ci_app_tests directory. I'm happy to help if needed. |
Outstanding work:
|
@daboehme this PR is now ready for review. |
Hi @ilumsden, I added a few fixes in a PR to your branch. I can't run the tests via It works for me when I add the installed location to I got a type mismatch error with the I'd also like to move the sources to |
@daboehme thanks for the feedback. I've moved the bindings to I also fixed a few bugs in the test file where I was not using the Python API correctly. The test should now work. I'm currently messing with the GitHub Actions runner to see if I can get that to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like the github actions test failed because apt didn't find the python-pybind11 package.
The attribute properties should always be passed in as an int since we can combine multiple cali_attr_properties
flags.
Not sure if we should keep the variant in the Python interface. That's getting a bit too low level. Looks like you use it in the Annotation class and for constructing attributes with extra metadata. Do you have any specific use cases for these? If not I think we can take these out and minimize the interface.
PythonAttribute(const char *name, cali_attr_type type); | ||
|
||
PythonAttribute(const char *name, cali_attr_type type, | ||
cali_attr_properties opt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opt
argument should be an int. That's because you can combine multiple cali_attr_properties
flags via OR
. Same in all other functions that take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the C++ side, cali_attr_properties
is an unscoped enum. As a result, per the C++ standard, it acts as an implementation-defined integral with size less than or equal to int
. That means it's perfectly acceptable to apply an OR
to multiple cali_attr_properties
values.
Beyond the C++ stuff, it's actually better to use cali_attr_properties
for the Python bindings because pybind11 does some auto-conversion stuff between Python and C++ types that could get messy if I just accepted an int
.
One alternative that would play nice with everything is to export the properties as an int
in pybind11 and then wrap the values in a proper Python enum
. In that case, the Python interface could be a subclass of enum.IntFlag
, which would provide a native way of doing OR
logic in Python that would still convert to int
when passed to C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore my comment regarding IntFlag
. That won't really be possible because Python does not support method overloading.
Looks like |
My last commit fixes that. |
Oh, wait. The CPP file. Give me a sec to fix that. |
Ok. Everything should be working with this new commit. I just tested it on Corona. |
…ltiple constructors
@daboehme now that CI is passing, I just rebased this branch. It should now be ready for "final" review and (hopefully) merge. |
This PR adds initial Python bindings to Caliper using the pybind11 library. The resulting Python library (named
pycaliper
) contains bindings for the following Caliper APIs:cali::ConfigManager
cali::Variant
cali::Annotation
cali_attribute
functionscali_begin_*
functionscali_set_*
functionscali_end
functioncali_*_byname
functionscali_[begin | end]_region
functionscali_[begin | end]_comm_region
functionscali_[begin | end]_phase
functionscali_set_global_*_byname
functionscali_make_loop_iteration_attribute
cali_init
cali_is_initialized
cali_attr_type
enumcali_attr_properties
enumThese bindings have been tested using the annotations added to Hatchet and Thicket in LLNL/hatchet#142 and LLNL/thicket#195.
Remaining Work:
These bindings are complete and could be merged as-is. However, it would be ideal if I added docstrings to the Python module before merge.