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.

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_as_int__ for >10 minor/patch release vers (resolves #1982) #1993

Merged

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

+ (10 * int(version_split[1]))
+ (1 * int(version_split[2]))
_version_split = __version__.split(".")
__version_info__ = tuple(map(int, _version_split))
Copy link
Contributor

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-opentensor ibraheem-opentensor 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
12 checks passed
@gus-opentensor gus-opentensor 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