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"?
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?
> 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).
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.
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.
> 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.
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 :)
Samuel
