easybuild-framework icon indicating copy to clipboard operation
easybuild-framework copied to clipboard

replaced distutils LooseVersion with packaging.version.parse

Open robogast opened this issue 3 years ago • 5 comments

Fixes #4072, and #3996 partially.

This is my first PR for easybuild-framework, help is appreciated :)

robogast avatar Sep 08 '22 15:09 robogast

Adding an extra requirement is quite a lot to ask. I implemented a simple copy of LooseVersion in #3794 and is purposely simple.

And I mean from #4072

comparing Java-1.8_265-b01 to Java-1.8.0_292, i.e. 1.8_265 to 1.8.0_292

What does it really mean to compare 1.8_265 to 1.8.0_292? Without someone checking those shouldn't be (easily) comparable so an error in this case is good IMO.

Flamefire avatar Sep 20 '22 07:09 Flamefire

The pros/cons for the two ways of resolving the version comparison problem (feel free in adding/subtracting pros/cons from both lists, the following is just my opinion)

packaging.version.parse:

Pros:

  • Explicitly adheres to Python's extensive and well defined versioning scheme (PEP-0440)
  • Available for all Python versions (including 2.7)
  • Is the advised way of replacing disutils.version.LooseVersion (PEP-0632)
    • packaging is already included in setuptools
    • distutils (which is used in e.g. setup.py) is deprecated and needs to be replaced by setuptools anyway by Python 3.12
    • PEP-0632 is also linked in the thread from which @Flamefire takes the fix for LooseVersion's str/int comparison issue #3996.

Cons:

  • Adds a requirement
  • Makes a (possibly wrong) decision in ambiguous cases such as comparing Java-1.8_265-b01 to Java-1.8.0_292, i.e. 1.8_265 to 1.8.0_292

easybuild-framework.tools.LooseVersion

Pros:

  • No additional requirement
  • "Simple" implementation

Cons:

  • Doesn't adhere to any standard(ized) versioning scheme
  • Re-invents the wheel
  • Doesn't handle complex cases (automatically)

I understand why the ambiguous case of comparing 1.8_265 to 1.8.0_292 is unfortunate, but breaking tools such as tweak.py in favor of manual decision making is (in my opinion) not a solution.

robogast avatar Sep 20 '22 14:09 robogast

We need to also consider the easyblocks and the many many places they use LooseVersion (and not only the ones we have, but also whatever people are using outside of the main repo). SI suspect noone is volunteering to changing all of those to parse and checking that nothing broke, so a switch to parse is something i think we have to introduce gradually there, and we regardless need to provide the easybuild-framework.tools.LooseVersion during that transition.

Micket avatar Sep 22 '22 10:09 Micket

My 2 cents on this: while it seems like we may not be able to avoid a dependency on setuptools from Python 3.12 onwards (to be checked, I still hope we can avoid that since we know it's going to be a PITA if not), my preference goes to "vendoring" LooseVersion as is done in #3794.

Not only does it avoid introducing a dependency on a Python package outside the standard library, but it also means that nothing really changes, so there won't be surprises with version checking suddenly being done slightly differently.

We have a broad set of test cases in the easyconfigs repository that we can test with, there are other interesting cases like OpenFOAM, LAMMPS, etc. which we should also check.

That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).

boegel avatar Oct 12 '22 07:10 boegel

That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).

I don't see how #3794 is a breaking change. It is a more or less 1:1 copy of the existing LooseVersion with a corner case fixed which didn't work before simplifying code that wants to use it. I would suggest to merge that and transition to it soon so that we can test it extensively before the next release.

Flamefire avatar Oct 12 '22 08:10 Flamefire

That said, I think whatever road we take, this is EasyBuild 5.0 material, since the potential impact is not small (and Python 3.12 is not available yet).

I don't see how #3794 is a breaking change. It is a more or less 1:1 copy of the existing LooseVersion with a corner case fixed which didn't work before simplifying code that wants to use it. I would suggest to merge that and transition to it soon so that we can test it extensively before the next release.

I didn't call #3794 a breaking change, but it definitely is an intrusive one. In theory, it's the same code (except, it's not really, since there's a bugfix on top).

We've discussed #3794 briefly during the EasyBuild conf call (again) today, and the general consensus is that we should just go ahead with it (without waiting for EasyBuild v5.0).

boegel avatar Oct 26 '22 17:10 boegel

Closing this since #3794 has been merged

boegel avatar Dec 21 '22 21:12 boegel