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

replaced distutils LooseVersion with packaging.version.parse #4074

Closed
wants to merge 1 commit into from

Conversation

robogast
Copy link

@robogast robogast commented Sep 8, 2022

Fixes #4072, and #3996 partially.

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

@Flamefire
Copy link
Contributor

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.

@robogast
Copy link
Author

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)

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.

@Micket
Copy link
Contributor

Micket commented Sep 22, 2022

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.

@boegel boegel added the change label Oct 12, 2022
@boegel boegel added this to the 4.x milestone Oct 12, 2022
@boegel boegel changed the title replaced distutils LooseVersion with packaging parse replaced distutils LooseVersion with packaging.version.parse Oct 12, 2022
@boegel
Copy link
Member

boegel commented Oct 12, 2022

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).

@Flamefire
Copy link
Contributor

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.

@boegel
Copy link
Member

boegel commented Oct 26, 2022

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
Copy link
Member

boegel commented Dec 21, 2022

Closing this since #3794 has been merged

@boegel boegel closed this Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug|feature] Fixing version parsing of easyconfigs in tweak.py
4 participants