LLNL / py-hostlist

hostlist utility implemented in python
MIT License
6 stars 2 forks source link

Fixing issue in compress_range funciton #30

Closed morenodelgad1 closed 3 years ago

morenodelgad1 commented 3 years ago

The initial implementation of compress_range only checked if there was a "0" in the whole string (not position based) which caused zeroes to be added to host-ranges when not explicitly invoked. The suggest fix strips the hostname and leaves node node numbers in tact (host117 -> 117) and then checks if the first number if a "0".

codecov-io commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@90d6867). Click here to learn what that means. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #30   +/-   ##
=========================================
  Coverage          ?   97.45%           
=========================================
  Files             ?        2           
  Lines             ?      669           
  Branches          ?        0           
=========================================
  Hits              ?      652           
  Misses            ?       17           
  Partials          ?        0           
Impacted Files Coverage Δ
hostlist/hostlist.py 95.46% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 90d6867...4d18ce5. Read the comment docs.

cmoussa1 commented 3 years ago

Thanks for issuing this fix @morenodelgad1! This is a good change. I pulled down your branch and tested it with the hostlist string we tried yesterday and confirmed it works:

>>> hostlist.compress_range('corona208,corona209,corona211,corona149,corona150,corona151,corona152,corona153,corona154,corona155,corona156,corona157,corona158,corona159,corona160,corona161,corona162,corona163,corona164,corona165,corona166,corona167,corona168,corona169,corona170,corona171,corona172,corona173,corona174,corona175,corona176,corona177,corona178,corona179,corona180,corona181,corona182,corona183,corona184,corona185,corona186,corona187,corona188,corona189,corona190,corona191,corona192,corona193,corona194,corona195,corona196,corona197,corona198,corona199,corona200,corona201,corona202,corona203,corona204,corona205,corona206,corona207,corona213,corona214,corona215,corona216,corona217,corona218,corona219,corona220,corona221,corona222,corona223,corona224,corona225,corona226,corona227,corona228,corona229,corona230,corona231,corona232,corona233,corona234,corona235,corona236,corona237,corona238,corona239,corona240,corona241,corona242,corona243,corona244,corona245,corona246,corona247,corona248,corona249,corona250,corona251,corona252,corona253,corona254,corona255,corona256,corona257,corona258,corona259,corona260,corona261,corona262,corona263,corona264,corona265,corona266,corona267,corona268,corona269,corona270,corona271,corona272,corona273,corona274,corona275,corona276,corona277,corona278,corona279,corona280,corona281,corona282,corona283,corona284,corona285,corona286,corona287,corona288,corona289,corona290,corona291,corona292,corona293,corona294,corona295')
'corona[149-209,211,213-295]'
>>> 

And all of the unit tests pass:

moussa1@trinity1 ~/src/py-hostlist/hostlist$ python unittest_hostlist.py 
...............................................................
----------------------------------------------------------------------
Ran 63 tests in 0.006s

OK

So I will approve these changes. But first:

It looks like the Travis build failed with Python 3.3 (which I can't remember why I was checking all of these different Python versions in the first place - they are probably not necessary, right?)

Screen Shot 2021-01-05 at 12 48 53 PM

But this should probably be its own fix in a separate PR. Perhaps I should post a small PR that removes the versions of Python from .travis.yml we don't want to build against (maybe we can just build against 3.6, 3.6-dev, and 3.7-dev? I'll defer to you to determine which versions are necessary to keep.

morenodelgad1 commented 3 years ago

I don't believe we care about Python 3.3. All of the systems that will be using py-hostlist will be at least 3.6 and above. I would recommend keeping versions 3.5 and above.

cmoussa1 commented 3 years ago

Sounds good. I will post a .travis.yml PR that removes the checks for unneeded versions.

cmoussa1 commented 3 years ago

Thanks for working on this @morenodelgad1! Will merge now.