Skip to content

fix __version_as_int__ for >10 minor/patch release vers (resolves #1982)#1993

Merged
thewhaleking merged 2 commits intoopentensor:stagingfrom
backend-developers-ltd:fix_version_as_int
Jun 11, 2024
Merged

fix __version_as_int__ for >10 minor/patch release vers (resolves #1982)#1993
thewhaleking merged 2 commits intoopentensor:stagingfrom
backend-developers-ltd:fix_version_as_int

Conversation

@mjurbanski-reef
Copy link
Contributor

Bug

#1982

Description of the Change

Uses 1000 base, to fix 6.12.2 __version_as_int__ being larger than one from release 7.2.1 .

Alternate Designs

already discussed in #1982

Possible Drawbacks

If for some reason int16 was used to store the version number it could be a problem, but glance at subtensor code seems to indicate, int32 was used there.

Verification Process

Manual + added simple asserts to prevent similar problems from ever occurring in the future (it will basically immediately explode on import)

Release Notes

  • Fix bittensor.__version_as_int__ from release 6.12.x being higher number than one from 7.x line by encoding it using 1000 base instead of 10.
  • Deprecated bittensor.version_split - use bittensor.__version__, bittensor.__version_info__ or bittensor.__version_as_int__ instead

@ibraheem-abe ibraheem-abe requested a review from a team June 10, 2024 19:16
+ (10 * int(version_split[1]))
+ (1 * int(version_split[2]))
_version_split = __version__.split(".")
__version_info__ = tuple(map(int, _version_split))
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use

__version_info__ = tuple(int(part) for part in _version_split)

instead of

__version_info__ = tuple(map(int, _version_split))

in this example it is the same in terms of optimization, but code readability is much better

Copy link
Contributor

@ibraheem-abe ibraheem-abe Jun 10, 2024

Choose a reason for hiding this comment

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

@RomanCh-OT I can make this change when this gets merged. Just waiting for confirmation from subtensor side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied;

as for version being u32, AFAIK, that was needed to be checked was in
https://github.com/opentensor/subtensor/blob/0c77062a10cadfbab8507b7dccb2e1ea49848e58/pallets/subtensor/src/serving.rs#L57C9-L57C16
even with prometheus is u32 , but that version number is arbitrary set by node so it doesn't even matter in our context
https://github.com/opentensor/subtensor/blob/0c77062a10cadfbab8507b7dccb2e1ea49848e58/pallets/subtensor/src/serving.rs#L160

So it should be safe.

@thewhaleking thewhaleking merged commit a99a9bd into opentensor:staging Jun 11, 2024
@thealligatorking thealligatorking mentioned this pull request Jun 12, 2024
@thewhaleking thewhaleking linked an issue Jun 13, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bitensor version as int supports only minor,patch versions <10

4 participants