BUG: _compare_version backward incompat

[larsoner]

cc @hoechenberger:

>>> import mne
>>> mne.fixes._compare_version('1.0.dev0', '>=', '1.0')
False
>>> from distutils.version import LooseVersion
>>> LooseVersion('1.0.dev0') >= LooseVersion('1.0')
<stdin>:1: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
True

This is a problem because we usually want the second result :(

The git grep check_version | wc -l count is 67 and git grep _compare_version | wc -l is 26, I'd rather not have to change all of these to allow for .dev0 extensions. Plus other submodules like mne-nirs that already use this (where I saw the problem actually!) will not work properly...

I'm tempted to change this to cut off any .dev from the end to make life easy, but then we depart from standard. But it seems a bit like the lesser of two evils here.

[hoechenberger]

I believe there were some relatively recent changes by @cbrnr if I'm not mistaken (cannot check right now), could you have a look at the history of that function and see if this problem maybe only started showing up after these changes?

[cbrnr]

This is a bit unfortunate and maybe even a bug? They are explicitly recommending to switch to packaging.version.parse, but it doesn't behave identically. I've open an issue.

[hoechenberger]

I have to say that IMHO the new behavior is more correct than the old one though. 1.0 is newer than 1.0dev0. So the version comparison should be >=1.0dev0, no? I think this is how conda behaves too if I'm not mistaken.

[cbrnr]

Technically yes, dev0 is a prerelease, so it is less than the final release. However, speaking of semantic versioning, I think we're not using those prerelease tags correctly, because they should be separated by a hyphen (e.g. 1.0.0-dev0).

[cbrnr]

As @hoechenberger suspected this is expected behavior, and distutils.version was doing its own thing. I'd change the comparisons in our code. Alternatively, we could vendor the old versioning code, but I'd rather not do that.

[hoechenberger]

Let's change our code to make it more consistent with my thinking 😅🥴😁

[larsoner]

I'd rather add a loose=True that prunes the dev. Most of the time if a person is using 1.0.dev0, they probably have what's needed and if they don't they know how to update. Plus it's backward compatible this way. And it's much easier than changing 100+ places we use (and think about) these comparisons.

[cbrnr]

So this would basically affect equality comparisons, right? Using loose=True would mean that 1.0.dev0 >= 1.0, 1.0.dev0 <= 1.0, and 1.0.dev0 == 1.0 are all True. Does this make sense? I don't really understand why we rely on such strange comparisons.

[larsoner]

I can document this inaccuracy with == and loose=True. In practice we (almost?) never use ==. To me practicality -- and backward compat for other libs and even users that make use of check_version (and have only recently silently been getting the wrong result with parse, at least according to how check_version has behaved for the last 9 years) -- beats purity here by a long shot.

[cbrnr]

Even if documented just stripping the dev0 suffix doesn't make sense – this doesn't restore the old behavior, which is:

>>>  LooseVersion("1.0.dev0") >= LooseVersion("1.0")
True
>>>  LooseVersion("1.0.dev0") <= LooseVersion("1.0")
False
>>>  LooseVersion("1.0.dev0") == LooseVersion("1.0")
False

So the difference is that a dev0 version was newer previously, but is now considered older.

[larsoner]

The most important thing is that it restores the behavior of check_version. This uses _compare_version under the hood with a < operator to say if something is not okay:

mne-python/mne/utils/check.py

Line 100 in 6608b72

if _compare_version(version, '<', min_version):

So for example:

>>> LooseVersion('1.0.dev0') < LooseVersion('1.0')
False
>>> mne.fixes._compare_version('1.0.dev0', '<', '1.0')
True

So I think it should work. So maybe we should add loose=True to check_version, do the stripping there, and not change _compare_version (and just fix the way fewer of those we use, where needed).

[cbrnr]

So maybe we should add loose=True to check_version, do the stripping there, and not change _compare_version (and just fix the way fewer of those we use, where needed).

This sounds good!