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
