python: add bindings for libflux-idset,hostlist #3341

grondo · 2020-11-14T17:57:24Z

This PR is pretty straightforward. It adds basic IDset and Hostlist Python classes which wrap libflux-idset.so and libflux-hostlist.so.

I was also going to make automated bindings for librlist to make an Rv1 class, however I decided to stop here and make the Rv1 class a separate PR.

lgtm-com · 2020-11-14T18:11:17Z

This pull request introduces 3 alerts when merging 26b22c2 into 5945183 - view on LGTM.com

new alerts:

2 for __iter__ method returns a non-iterator
1 for Missing call to __del__ during object destruction

grondo · 2020-11-14T21:40:22Z

Force-pushed fixes for the python errors caught by LGTM.

garlick · 2020-11-16T14:44:27Z

Just playing with this a little. I can't seem make hostlist.decode() raise an exception on bad input. Instead I get a zero length hostlist. Is that expected?

grondo · 2020-11-16T15:01:21Z

No, and I thought I had tests for that but I must have accidentally dropped them at some point.

Nice catch. I'll work on a fix.

lgtm-com · 2020-11-16T18:12:13Z

This pull request introduces 1 alert when merging 5132efb into 02ee7a1 - view on LGTM.com

new alerts:

1 for Unused local variable

chu11 · 2020-11-16T18:40:38Z

src/bindings/python/flux/hostlist.py

@@ -167,6 +201,6 @@ def copy(self):
        return Hostlist(handle=self.pimpl.copy())


-def decode(string):
+def decode(arg):
    """Decode a string in RFC 29 hostlist format to a Hostlist object"""


should this comment be updated to string or iterable?

Yeah, thanks. I'm actually not sure of Python idioms, if hostlist.decode() just directly calls hostlist.Hostlist() is it even useful to separately provide that function?

good point. can't speak for whats common, but since there is an encode, i think it's nice to have a decode, minimally so people can search for it.

Ah good point! It doesn't do any harm at least...

grondo · 2020-11-16T19:12:34Z

Ok, pushed some fixup commits to address comments and improve interface and tests:

Fixed missing exceptions on hostlist_decode(3) failures, added tests
Improved Hostlist() constructor, now takes any valid RFC 29 hostlist string or iterable of said strings
Improved hostlist.append() to take Hostlist, or iterable or plain string of RFC 29 strings
add a few more tests

garlick

Nice! That fixed my problem and offhand I'm not finding anything else.
It's really cool to be able to use these "natively" in python!

I'll give my approval although someone with stronger python-fu like @chu11 probably ought to give a nod also.

grondo · 2020-11-16T19:37:01Z

Thanks. As I noted above in a comment, I'm not sure if the separate hostlist.decode() and idset.decode() functions are the "pythonic" way to do it, or if people expect to just use the class constructors anyway.

I was going to stop here for now and then see if an extra interfaces or minor interface changes popped out of work in Python-based flux utilities like flux-resource.py, and add those at that time.

grondo · 2020-11-16T19:42:09Z

Ok, perhaps I'll squash the fixup commits unless there is any objection?

chu11

LGTM, but my python-fu is probably not much better than @garlick

grondo · 2020-11-16T21:32:15Z

Actually, I'm not sure the approach used here (idset and hostlist each under their own ffi instance) will work.

The Rv1 or similar implementation will need an ffi instance that includes both the idset and hostlist instances, and I'm not exactly sure how to do that, but I wonder if what we really want here is to build one cffi source for all of hostlist, idset, and rlist, or even just put these directly into the _core.c ffi?

I really don't know anything about cffi yet, so I wonder if either @trws or @SteVwonder can give a hint on what might be the right approach here. Note that the use case is that an Rv1 class would have methods to return Hostlist or IDset objects directly from Rv1.nodelist or Rv1.ranks methods or properties. (These methods would call the underlying rlist_nodelist(struct rlist *rl) call and then instantiate a Hostlist with something like Hostlist(handle=result)

Right now when I attempt that I get the error:

TypeError: initializer for ctype 'struct hostlist *' appears indeed to be 'struct hostlist *', but the types are different (check that you are not e.g. mixing up different ffi instances)

grondo · 2020-11-16T22:29:22Z

Ah, just after the call I found that using ffi.include() "seems" to work, e.g. in the new _idset_build.py the hostlist and idset FFIs are included via

from _hostlist_build import ffi as hostlist_ffi
from _idset_build import ffi as idset_ffi

ffi = FFI()

ffi.include(hostlist_ffi)
ffi.include(idset_ffi)

Then I am able to instantiate Hostlist and IDset objects in the rlist.py module with simply:

   def nodelist(self):
        return Hostlist(handle=self.pimpl.nodelist())

    def ranks(self):
        return IDset(handle=self.pimpl.ranks())

grondo · 2020-11-16T22:50:27Z

Ok, since there is a path forward for composing FFI objects from multiple sources, I think this one is ok to go in as is. We can add minor tweaks as we come up with users of these new classes.

I added one more commit here. I noticed the asan builder was timing out when it seemed like the build would have completed given more time. Perhaps with the addition of extra tests the 40m timeout was not quite enough, so I've added a commit to increase the timeout to 50m.

garlick · 2020-11-16T23:40:10Z

Sounds good! Set MWP?

trws · 2020-11-16T23:42:12Z

This is exactly what you want here I think. If they’re included the ffi objects and type-identifiers are cross-compatible. We used to do this for several flux core components before re-working the build to build all of the standard core components as one module.

…

On 16 Nov 2020, at 14:29, Mark Grondona wrote: Ah, just after the call I found that using `ffi.include()` "seems" to work, e.g. in the new `_idset_build.py` the `hostlist` and `idset` FFIs are included via ```python from _hostlist_build import ffi as hostlist_ffi from _idset_build import ffi as idset_ffi ffi = FFI() ffi.include(hostlist_ffi) ffi.include(idset_ffi) ``` Then I am able to instantiate `Hostlist` and `IDset` objects in the `rlist.py` module with simply: ```python def nodelist(self): return Hostlist(handle=self.pimpl.nodelist()) def ranks(self): return IDset(handle=self.pimpl.ranks()) ``` -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3341 (comment)

SteVwonder

Thanks @grondo for putting this together. LGTM! Two totally optional comments below:

src/bindings/python/flux/hostlist.py

grondo · 2020-11-17T22:30:10Z

Ok, I've pushed an update that replaces the use of range()[index] with a new rangeslice() function, and also removed the unnecessary expand method as suggested by @SteVwonder.

codecov-io · 2020-11-17T23:00:26Z

Codecov Report

Merging #3341 (538173f) into master (0e51db1) will increase coverage by 0.11%.
The diff coverage is 97.98%.

@@            Coverage Diff             @@
##           master    #3341      +/-   ##
==========================================
+ Coverage   81.84%   81.96%   +0.11%     
==========================================
  Files         298      300       +2     
  Lines       46146    46394     +248     
==========================================
+ Hits        37767    38025     +258     
+ Misses       8379     8369      -10

Impacted Files	Coverage Δ
src/bindings/python/flux/idset.py	`97.97% <97.97%> (ø)`
src/bindings/python/flux/hostlist.py	`98.00% <98.00%> (ø)`
src/modules/job-info/guest_watch.c	`75.00% <0.00%> (-1.15%)`	⬇️
src/broker/broker.c	`73.97% <0.00%> (+0.11%)`	⬆️
src/broker/module.c	`75.62% <0.00%> (+0.45%)`	⬆️
src/broker/state_machine.c	`82.18% <0.00%> (+0.53%)`	⬆️
src/modules/job-archive/job-archive.c	`59.68% <0.00%> (+0.79%)`	⬆️
src/common/libhostlist/hostlist.c	`97.74% <0.00%> (+0.90%)`	⬆️
... and 2 more

SteVwonder · 2020-11-18T20:14:36Z

LGTM! Ready for the MWP label?

grondo · 2020-11-18T20:26:42Z

Ok, sure, thanks!

grondo · 2020-11-18T20:58:43Z

Ah, this will have to be manually merged since I've modified .github/workflows

Let me rebase first.

Problem: There is no Python interface for RFC29 hostlist format, which is now used in Rv1 in flux-core. Make a simple Hostlist class which wraps Flux's libhostlist, so that Python programs can operate on hostlists with exact compatibility with C code. Fixes flux-framework#3250

Problem: There is no Python interface for RFC22 Idset encoding. Add a simple Python IDset class which wraps libidset so that Python programs can encode/decode and manipulate idsets with full compatibility with the C library.

Add unit tests for the Python hostlist bindings

Add unit tests for the Python idset bindings.

Problem: The cffi emit_c_code() function does not update the target if it believes the generated .c file is already up-to-date. This causes make confusion because the file is never updated if one of its other prerequisites changes (e.g. Makefile), so the rule will continually be rerun on each make invocation. Ensure the target file mtime is updated in the *_build.py scripts by "touching" the file after ffi.emit_c_code() is complete. This way make won't unnecessarily re-run the rule to update the target.

Wrap rules in src/bindings/python/_flux/Makefile.am in necessary automake variables to make the output quiet by default. As with the rest of the build system `make V=1` will restore the extra-chatty build.

Problem: mypy complains about the _flux.* generated bindings with the error "Cannot find implementation or library stub for module". Add 'ignore_missing_imports' for _flux._hostlist and _flux._idset.

Problem: As we add more tests to the full `make check`, the ASan github workflow build is taking longer. The current time limit is 40m, but sometimes this is not quite long enough. Add another 10 minutes to the asan builder timeout to avoid premature timeouts of this workflow run.

SteVwonder · 2020-11-18T21:48:52Z

All the tests pass, so I'm going to push the button. Thanks @grondo!

grondo · 2020-11-18T22:06:40Z

Thanks!!

grondo force-pushed the python-hostlist-idset branch from 26b22c2 to 7b6a3a1 Compare November 14, 2020 21:39

grondo force-pushed the python-hostlist-idset branch from ec02323 to 5132efb Compare November 16, 2020 17:58

grondo force-pushed the python-hostlist-idset branch 2 times, most recently from 1069ec8 to bad84d9 Compare November 16, 2020 18:27

chu11 reviewed Nov 16, 2020

View reviewed changes

grondo force-pushed the python-hostlist-idset branch from bad84d9 to 64f0e24 Compare November 16, 2020 19:10

garlick approved these changes Nov 16, 2020

View reviewed changes

grondo requested review from SteVwonder and chu11 November 16, 2020 19:37

chu11 approved these changes Nov 16, 2020

View reviewed changes

grondo force-pushed the python-hostlist-idset branch 2 times, most recently from 96e9929 to 5099f14 Compare November 16, 2020 20:08

chu11 mentioned this pull request Nov 16, 2020

implement flux hostlist command #3344

Closed

SteVwonder approved these changes Nov 17, 2020

View reviewed changes

src/bindings/python/flux/hostlist.py Show resolved Hide resolved

src/bindings/python/flux/hostlist.py Show resolved Hide resolved

grondo force-pushed the python-hostlist-idset branch from b999641 to 7952de7 Compare November 17, 2020 22:29

grondo force-pushed the python-hostlist-idset branch from 7952de7 to 538173f Compare November 17, 2020 22:36

grondo added the merge-when-passing label Nov 18, 2020

grondo added 9 commits November 18, 2020 13:11

build: python: cleanup _flux/Makefile.am formatting

b44e06d

python: add flux.idset binding

e101b78

Problem: There is no Python interface for RFC22 Idset encoding. Add a simple Python IDset class which wraps libidset so that Python programs can encode/decode and manipulate idsets with full compatibility with the C library.

testsuite: add t0020-hostlist.py

56585eb

Add unit tests for the Python hostlist bindings

testsuite: add t0021-idset.py

b5447ac

Add unit tests for the Python idset bindings.

build: python: quiet the build of auto-generated bindings

e788bfc

Wrap rules in src/bindings/python/_flux/Makefile.am in necessary automake variables to make the output quiet by default. As with the rest of the build system `make V=1` will restore the extra-chatty build.

mypy: ignore missing imports for _flux.{hostlist,idset}

efd445d

Problem: mypy complains about the _flux.* generated bindings with the error "Cannot find implementation or library stub for module". Add 'ignore_missing_imports' for _flux._hostlist and _flux._idset.

grondo force-pushed the python-hostlist-idset branch from 538173f to e7bd68a Compare November 18, 2020 21:11

SteVwonder merged commit f1ce3dc into flux-framework:master Nov 18, 2020

grondo deleted the python-hostlist-idset branch November 18, 2020 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python: add bindings for libflux-idset,hostlist #3341

python: add bindings for libflux-idset,hostlist #3341

grondo commented Nov 14, 2020

lgtm-com bot commented Nov 14, 2020

grondo commented Nov 14, 2020

garlick commented Nov 16, 2020

grondo commented Nov 16, 2020

lgtm-com bot commented Nov 16, 2020

chu11 Nov 16, 2020

grondo Nov 16, 2020

chu11 Nov 16, 2020

grondo Nov 16, 2020

grondo commented Nov 16, 2020

garlick left a comment

grondo commented Nov 16, 2020

grondo commented Nov 16, 2020

chu11 left a comment

grondo commented Nov 16, 2020 •

edited

Loading

grondo commented Nov 16, 2020

grondo commented Nov 16, 2020

garlick commented Nov 16, 2020

trws commented Nov 16, 2020 via email

SteVwonder left a comment

grondo commented Nov 17, 2020

codecov-io commented Nov 17, 2020

SteVwonder commented Nov 18, 2020

grondo commented Nov 18, 2020

grondo commented Nov 18, 2020

SteVwonder commented Nov 18, 2020

grondo commented Nov 18, 2020

python: add bindings for libflux-idset,hostlist #3341

python: add bindings for libflux-idset,hostlist #3341

Conversation

grondo commented Nov 14, 2020

lgtm-com bot commented Nov 14, 2020

grondo commented Nov 14, 2020

garlick commented Nov 16, 2020

grondo commented Nov 16, 2020

lgtm-com bot commented Nov 16, 2020

chu11 Nov 16, 2020

Choose a reason for hiding this comment

grondo Nov 16, 2020

Choose a reason for hiding this comment

chu11 Nov 16, 2020

Choose a reason for hiding this comment

grondo Nov 16, 2020

Choose a reason for hiding this comment

grondo commented Nov 16, 2020

garlick left a comment

Choose a reason for hiding this comment

grondo commented Nov 16, 2020

grondo commented Nov 16, 2020

chu11 left a comment

Choose a reason for hiding this comment

grondo commented Nov 16, 2020 • edited Loading

grondo commented Nov 16, 2020

grondo commented Nov 16, 2020

garlick commented Nov 16, 2020

trws commented Nov 16, 2020 via email

SteVwonder left a comment

Choose a reason for hiding this comment

grondo commented Nov 17, 2020

codecov-io commented Nov 17, 2020

Codecov Report

SteVwonder commented Nov 18, 2020

grondo commented Nov 18, 2020

grondo commented Nov 18, 2020

SteVwonder commented Nov 18, 2020

grondo commented Nov 18, 2020

grondo commented Nov 16, 2020 •

edited

Loading