Open MPI logo

Hardware Locality Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Hardware Locality Development mailing list

Subject: Re: [hwloc-devel] python bindings
From: Guy Streeter (streeter_at_[hidden])
Date: 2011-01-20 13:02:49


On 01/20/2011 11:32 AM, Samuel Thibault wrote:
> Hello,
>
> Guy Streeter, le Mon 17 Jan 2011 21:03:04 +0100, a écrit :
>> I am currently working to get a public git repository set up so that I can
>> share the work. In the meantime, my first pass at python bindings for hwloc
>> are available from
>>
>> http://people.redhat.com/streeter/
>
> Here are some comments on some bits of code:
>
>> topo = hwloc.hwloc_topology()
>> assert obj.type == hwloc.HWLOC_OBJ_PU
>> orig = hwloc.hwloc_bitmap.alloc()
>
> Mmm, why repeating "hwloc"?

I think removing the "hwloc_" prefix from the class names is a good idea. I'll
do that.

Do you think I should also do it for those constants? I thought matching the C
constant name was better, but hwloc.OBJ_PU is less typing :)

Also, since they are class names, do you think I should capitalize them as is
common for python classes? "topo = hwloc.Topology()" ?

>
> It's nice to have rewritten the whole testsuite :) It's really useful to
> have a very good grasp on how it looks like.
>
>> hwlocset = topo.complete_cpuset.dup()
>
> Mmm, so if the user forgets to use dup(), he might still be able to
> write in a const cpuset?

I haven't figured out a way to mark things as non-modifiable. I'm still
researching it. Right now nothing stops you changing any bitmap.

>
>> def set_thread_cpubind(self, tid, cpuset):
>> """Bind a thread on cpus given in cpuset"""
>> return _linux_set_thread_cpubind(self, tid, cpuset)
>
> Mmm, no. If you use _linux_set_thread_cpubind, then call the python
> function linux_set_thread_cpubind, as this is using TIDs, which is a
> Linux invention. The normal set_thread_cpubind function should use the
> usual python thread identifier (just like hwloc's set_thread_cpubind
> uses the usual C thread identifier: pthread_t). I see in some other code
> that you wonder how to create pthread in python. Then don't provide the
> function for now (rather than providing a function which does not take
> the proper thread identification type).

OK, I understand this now.

>
> Also, could you clearly separate functions which are not defined
> in hwloc.h itself? In the C interface, they are in a separate e.g.
> helpers.h file in order to express that these are really helpers only,
> and that the user doesn't need to know them all, everything can be done
> with the core functions from hwloc.h.

Do you mean physically separate in the source code, or are you talking about
providing a different way to access them? I know the source code could use
some cleanup and more comments.

>
> Also, for more clarity, maybe you should also separate functions by
> comments according to the doxygen groups in hwloc.h, see \defgroup
> hwlocality_api_version API version which only contains HWLOC_API_VERSION
> and hwloc_get_api_version for now, for instance. That should make
> review and maintenance quite easier.
>
>> assert b2.ulong() == 0xa0a
>
> I wonder whether ulong() and ulong(i) are useful for the python
> bindings: these are mostly useful for legacy interfaces which just use
> single integers. Python has unbound integers, so it's not really useful
> in that case :) One thing that might be useful however is b2.uint32()
> and b2.uint64(), I guess we should provide it at the C stage first.

I couldn't think of a use for them either. I tried to implement everything
that is documented in the man pages. There may be other things that don't make
sense.

>
>> alloc_membind doesn't make a lot of sense in python, since you really
>> can't tell the python interpreter to use the space.
>> I can't think of a way to use hwloc_set_area_membind* in python
>
> They should probably take a python object itself, but since I guess the
> python GC does what it wants with moving data... That might however
> be useful when e.g. somebody drives C computation code from a python
> fast-prototyping main loop.
>
>> _libhwloc = ctypes.CDLL(ctypes.util.find_library('hwloc'),
>> use_errno=True)
>
> Mmm, shouldn't this include the soname of the library? Else built
> bindings will break on ABI changes.
>
I planned to check the library version at run-time. That way I can provide
backward compatibility.
>
>
> Apart from these nasty details, I like the interface style, thanks for
> the nice contribution ! :)
>
>> b1 = hwloc.hwloc_bitmap.alloc('0xf')
>> b2 = hwloc.hwloc_bitmap.alloc(range(4))
>
> Yay :)
>
I'm adding more support for bitmap representations:

assert b3.intersects('0xf...f,0xfffffffd,0xfffffff9')
assert b1.compare_first(0x3)
assert b1.compare('3') < 0

--Guy