Expose class structure in libucxx.pxd
#372
Conversation
pentschev
added
feature request
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| cdef class UCXContext: | ||
| cdef: | ||
| shared_ptr[Context] _context | ||
| dict _config | ||
|
|
||
| cpdef dict get_config(self) | ||
|
|
||
| cdef shared_ptr[Context] get_ucxx_shared_ptr(self) nogil | ||
|
|
||
|
|
||
| cdef class UCXAddress: | ||
| cdef: | ||
| shared_ptr[Address] _address | ||
| size_t _length | ||
| ucp_address_t *_handle | ||
| string _string | ||
|
|
||
| cdef shared_ptr[Address] get_ucxx_shared_ptr(self) nogil |
There was a problem hiding this comment.
suggestion/question: What does this get_ucxx_shared_ptr setup buy us?
Is it reasonable to instead write:
cdef class UCXFoo:
cdef:
shared_ptr[Foo] c_obj
And just access c_obj directly?
There was a problem hiding this comment.
Since Cython lacks private/public member protection, the getter doesn't really prevent the user from doing something unsupported. However, IMO, a getter signals this is a public API that may be used without risk by the user, the lack of a setter signals it shouldn't be modified. Exposing c_obj directly, without the leading underscore, may give the impression that the object can be read and modified, which is definitely not the case.
Does that make sense to you or is it just mumbo-jumbo from my head? Perhaps we should document it better to make the intent clear, but so we should do for the entire Cython interface, which is currently very poorly/not at all documented.
test_getters.pyx
Outdated
| # assert <uint64_t>ptr.get() == context.ucxx_ptr | ||
|
|
||
|
|
||
| cdef get_context(UCXContext context): | ||
| # test_shared(context.get_ucxx_shared_ptr()) | ||
| assert <uintptr_t>context.get_ucxx_shared_ptr() == context.ucx_ptr | ||
|
|
||
|
|
||
| def test_context_getter(): | ||
| # cdef shared_ptr[Context] ucxx_shared_ptr | ||
|
|
||
| feature_flags = [ucx_api.Feature.WAKEUP, ucx_api.Feature.TAG] | ||
|
|
||
| context = ucx_api.UCXContext(feature_flags=tuple(feature_flags)) | ||
|
|
||
| get_context(context) | ||
|
|
||
| # test_shared(<UCXContext>context.get_ucxx_shared_ptr()) | ||
|
|
||
| # ucxx_shared_ptr = context.get_ucxx_shared_ptr() | ||
|
|
||
| # assert ucxx_shared_ptr.get() != NULL | ||
| # assert <uint64_t>ucxx_shared_ptr.get() == context.ucxx_ptr |
There was a problem hiding this comment.
Good catch. I committed those locally while doing some cleanup with the intent to remove them later but forgot to do that. Removed now in 960fab8 .
|
Getting tests done here is getting too complicated and I'd like to make the changes available for projects that currently need those changes. Due to that, I've removed the tests and infrastructure to #376 and will try to get this merged as soon as tests pass. |
|
/merge |
|
Thanks all, I'll continue pushing on #376 for the remaining changes. |
Expose class structure in
libucxx.pxd. This change is useful so that projects building on top of the Python interface can takeshared_ptr<ucxx::Component>objects and plug those objects into their C++ backends.Although it's not possible to define private attributes in a
cdef class, we attempt to mark them private with the leading underscore in their names, such asUCXContext._context, and then provide aUCXContext.get_ucxx_shared_ptrfor those members that are expected to be externally usable.Testing will be handled as part of #376.