-
Notifications
You must be signed in to change notification settings - Fork 14
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
BAI-1299 Python semantic versioning bugfix #1305
Changes from all commits
013b931
4753ca7
4d6e5c7
8eacf69
ad8223c
c17fea5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,8 +55,6 @@ def __init__( | |
if images is None: | ||
images = [] | ||
|
||
if isinstance(version, str): | ||
version = Version(version) | ||
self.version = version | ||
|
||
self.model_card_version = model_card_version | ||
|
@@ -285,7 +283,7 @@ def update(self) -> Any: | |
""" | ||
return self.client.put_release( | ||
self.model_id, | ||
str(self.version), | ||
str(self.__version_raw), | ||
self.notes, | ||
self.draft, | ||
self.files, | ||
|
@@ -302,16 +300,39 @@ def delete(self) -> Any: | |
|
||
return True | ||
|
||
@property | ||
def version(self): | ||
return self.__version_obj | ||
|
||
@version.setter | ||
def version(self, value): | ||
Comment on lines
+307
to
+308
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's best to make this private. Just to avoid any problematic updating. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done - alternative fix for this. If the user tries to set the version after it has already been defined, it will throw a BailoException. Making the attribute itself private caused larger issues in the code base and created a breaking change |
||
if ("_Release__version_obj" not in self.__dict__) and ("_Release__version_raw" not in self.__dict__): | ||
if isinstance(value, str): | ||
if value.startswith("v"): | ||
value = value[1:] | ||
version_obj = Version.coerce(value) | ||
elif isinstance(value, Version): | ||
version_obj = value | ||
else: | ||
raise TypeError("Provided version not of a supported type.") | ||
|
||
self.__version_obj = version_obj | ||
self.__version_raw = value | ||
else: | ||
raise BailoException( | ||
"Version attribute has already been set once. You must create a new Release object to create a new version, or use Model.create_release()." | ||
) | ||
|
||
def __repr__(self) -> str: | ||
return f"{self.__class__.__name__}({str(self)})" | ||
|
||
def __str__(self) -> str: | ||
return f"{self.model_id} v{self.version}" | ||
return f"{self.model_id} v{self.__version_obj}" | ||
|
||
def __eq__(self, other) -> bool: | ||
if not isinstance(other, self.__class__): | ||
return NotImplemented | ||
return self.version == other.version | ||
return self.__version_obj == other.__version_obj | ||
|
||
def __ne__(self, other): | ||
if not isinstance(other, self.__class__): | ||
|
@@ -321,22 +342,22 @@ def __ne__(self, other): | |
def __lt__(self, other): | ||
if not isinstance(other, self.__class__): | ||
return NotImplemented | ||
return self.version < other.version | ||
return self.__version_obj < other.__version_obj | ||
|
||
def __le__(self, other): | ||
if not isinstance(other, self.__class__): | ||
return NotImplemented | ||
return self.version <= other.version | ||
return self.__version_obj <= other.__version_obj | ||
|
||
def __gt__(self, other): | ||
if not isinstance(other, self.__class__): | ||
return NotImplemented | ||
return self.version > other.version | ||
return self.__version_obj > other.__version_obj | ||
|
||
def __ge__(self, other): | ||
if not isinstance(other, self.__class__): | ||
return NotImplemented | ||
return self.version >= other.version | ||
return self.__version_obj >= other.__version_obj | ||
|
||
def __hash__(self) -> int: | ||
return hash((self.model_id, self.version)) | ||
return hash((self.model_id, self.__version_obj)) |
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 would put this validation in
post_release
as well within the create classmethod just to prevent an unnecessary response when creating that endpoint.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 don't think we want to fully stop users from adding the 'v' using the
post_release
method, as the front and backend will allow this. It only presented an issue when creating theVersion()
object which is used to compareRelease()
objects etc, which is resolved with this PR whilst allowing the user to include the 'v' upon creation if they want to