Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python: improve Hostlist class indexing #4993

Merged
merged 4 commits into from
Mar 11, 2023

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Mar 11, 2023

This simple PR improves the indexing of Hostlist objects by

  • returning a new Hostlist when more than one host is indexed, e.g. by slice (before, a list of hosts was returned)
  • allowing any iterable to be used with a Hostlist index, most notably a tuple, e.g. hl[1,3,5] and an idset hl[ids].

Problem: When slicing a Hostlist object, a Python list is returned, but
it would be far more practical and efficient to return a new Hostlist
instead. Also, this follows the example of Python list slicing, since
indexing a list returns an element, but slicing returns a new list.

Update the Hostlist index method to return a new Hostlist when a
slice is used. Update the expand() method and one test accordingly.
Problem: It would be convenient to index a hostlist with an iterable,
e.g. a tuple of values (`hl[1,5]`) or an idset (`hl[ids]`), but this
is not currently supported.

Support indexing a Hostlist object with any iterable, as long as the
iterable only contains integral values.
Problem: No tests of the python Hostlist interface verify that
iterable indexing works.

Add a set of tests for iterable indexing, including indexing using
an IDset and a test that an invalid iterable raises the appropriate
exception.
Problem: The flux-resource utility has to iterate an idset to generate
a new Hostlist, but now Hostlist supports direct indexing via an idset.

Update the one case where a new Hostlist is constructed by idset in
the flux-resource code.
@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

Merging #4993 (de5c702) into master (e796e78) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4993      +/-   ##
==========================================
- Coverage   83.10%   83.09%   -0.01%     
==========================================
  Files         443      443              
  Lines       76176    76185       +9     
==========================================
+ Hits        63303    63309       +6     
- Misses      12873    12876       +3     
Impacted Files Coverage Δ
src/bindings/python/flux/hostlist.py 97.02% <100.00%> (-0.78%) ⬇️
src/cmd/flux-resource.py 96.83% <100.00%> (-0.01%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow that's really neat!

@grondo
Copy link
Contributor Author

grondo commented Mar 11, 2023

Thanks! I'll set MWP.

@mergify mergify bot merged commit 532f17e into flux-framework:master Mar 11, 2023
@grondo grondo deleted the hostlist-index branch March 12, 2023 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants