-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix version checks in SDK #5991
Conversation
cvat-sdk/cvat_sdk/core/client.py
Outdated
# Check for (major, minor) compatibility. | ||
# Micro releases and fixes do not affect API compatibility in general. | ||
upper_bound = pv.Version(f"{target.epoch}{target.major}.{target.minor + 1}") | ||
return target <= current < upper_bound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is slightly wrong, because it will calculate that 2.5.0a1 is compatible with 2.4.0. I would suggest something like this instead:
return current in packaging.specifiers.SpecifierSet(f"~= {target.epoch}!{target.major}.{target.minor}.{target.micro}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a nice catch.
Upd: it's better, but there is a problem with this approach, that
Version("2.1") in specifiers.Specifier(f"~= 2.1.pre1") -> True
Version("2.1.1") in specifiers.Specifier(f"~= 2.1.pre1") -> True
Version("2.1.1") in specifiers.Specifier(f"~= 2.1.0") -> True
Maybe it's not a big deal, but it doesn't look expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the version on the right side comes from the SDK, I wouldn't expect it to be a prerelease. And what's wrong with the 3rd case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, given what we do now (add the next release version to support the public instance, and once we publish it, we update SDK), it makes sense to try to limit the next version to its pre-release variant, so the old SDK will stop working automatically, once the new server release is deployed.
E.g.:
release 2.4
public server: 2.5.devX
sdk: v2.4 supporting >=2.4, <2.5
release 2.5
public server: 2.5
sdk v2.4 stops working
- Added missing compatibility with micro- and smaller releases
Motivation and context
How has this been tested?
Unit test
Checklist
develop
branch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.