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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 11, 2024
Merged

BAI-1299 Python semantic versioning bugfix #1305

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
013b931
Semantic versons containing 'v' will no longer break client
bw27492 Jun 3, 2024
4753ca7
Changed replace to removeprefix
bw27492 Jun 5, 2024
4d6e5c7
Fix for backwards compatibility to Python 3.8
bw27492 Jun 5, 2024
8eacf69
Fix to avoid unwanted updating of version attribute
bw27492 Jun 10, 2024
ad8223c
Fix to __dict__ conditional statement
bw27492 Jun 11, 2024
c17fea5
Merge branch 'main' into BAI-1299-python-semantic-version-bugfix
bw27492 Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions lib/python/src/bailo/helper/release.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,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
Expand Down Expand Up @@ -260,7 +258,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,
Expand All @@ -275,16 +273,33 @@ def delete(self) -> Any:
self.client.delete_release(self.model_id, str(self.version))
return True

@property
def version(self):
return self._version_obj

@version.setter
def version(self, value):
Copy link
Member

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.

Copy link
Member Author

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 the Version() object which is used to compare Release() objects etc, which is resolved with this PR whilst allowing the user to include the 'v' upon creation if they want to

Comment on lines +307 to +308
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@bw27492 bw27492 Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 isinstance(value, str):
version_obj = value.replace("v", "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces all v characters throughout the string. Which could be within the build or the pre-release. Instead use

version_obj = value.removeprefix("v")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member Author

@bw27492 bw27492 Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update - this was added in 3.9 so fails our Python 3.8 tests :( have changed to an older method

version_obj = Version.coerce(version_obj)
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's probably better to fetch from the object rather than the value. In this method coerce is used however this is not used on the backend validation. so will fail with strings like '0.1.2.3.4'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _version_raw attribute is the same value used to get/create releases on the server - so if the user inputs an incorrect value the server will return an error anyway. Also if we get the semver from the object, the v will have been removed so if the user calls .update() the server won't find the right release (e.g. the client will try to get 2.0.0 as opposed to v2.0.0)


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__):
Expand All @@ -294,22 +309,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))
12 changes: 10 additions & 2 deletions lib/python/tests/test_release.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def test_create_get_from_version_update_and_delete_release(


@pytest.mark.integration
def test_nonexistant_file_ids(integration_client, example_model):
def test_nonexistent_file_ids(integration_client, example_model):
with pytest.raises(ResponseException):
release = Release.create(
client=integration_client,
Expand All @@ -79,7 +79,7 @@ def test_nonexistant_file_ids(integration_client, example_model):


@pytest.mark.integration
def create_two_release_with_same_semver(integration_client, example_model):
def test_create_two_release_with_same_semver(integration_client, example_model):
Release.create(
client=integration_client, model_id=example_model.model_id, version="1.1.0", model_card_version=1, notes="test"
)
Expand All @@ -91,3 +91,11 @@ def create_two_release_with_same_semver(integration_client, example_model):
model_card_version=1,
notes="test",
)


@pytest.mark.integration
def test_retrieve_release_with_v_in_version(integration_client, example_model):
example_model.create_release(version="v2.0.0", notes=" ")
release = Release.from_version(client=integration_client, model_id=example_model.model_id, version="v2.0.0")

assert str(release.version) == "2.0.0"
Loading