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

Async for bittensor/extrinsics/registration.py #1996

Merged
merged 6 commits into from
Jun 10, 2024

Conversation

RomanCh-OT
Copy link
Contributor

@RomanCh-OT RomanCh-OT commented Jun 10, 2024

Part 3 of #1990

includes:

  • bittensor/extrinsics/prometheus.py
  • bittensor/extrinsics/registration.py

@RomanCh-OT RomanCh-OT requested a review from a team June 10, 2024 22:47
@RomanCh-OT RomanCh-OT self-assigned this Jun 10, 2024
@RomanCh-OT RomanCh-OT added feature new feature added bittensor labels Jun 10, 2024
@RomanCh-OT RomanCh-OT changed the base branch from master to merge-async June 10, 2024 22:47
@@ -659,7 +659,7 @@ class PrometheusInfo:

block: int
version: int
ip: str
ip: str # string representation of converted ip to int using tool like https://tools.iplocation.net/ip-to-integer-converter
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace the link to this one

https://netaddr.readthedocs.io/en/latest/api.html#netaddr.IPAddress.value

We actually use netaddr to do this in the networking.py file. This is their API documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

success (bool):
Flag is ``true`` if extrinsic was finalized or uncluded in the block.
If we did not wait for finalization / inclusion, the response is ``true``.
success (bool): Flag is ``true`` if extrinsic was finalized or uncluded in the block. If we did not wait for finalization / inclusion, the response is ``true``.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 'included' - spelling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

wallet.hotkey.ss58_address, netuid=netuid
)
neuron_up_to_date = not neuron.is_null and call_params == {
"version": neuron.prometheus_info.version,
"ip": net.ip_to_int(neuron.prometheus_info.ip),
"ip": neuron.prometheus_info.ip,
Copy link
Contributor

@opendansor opendansor Jun 10, 2024

Choose a reason for hiding this comment

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

does neuron.prometheus_info.ip still return an 'int'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to replace this back. done

@@ -469,7 +469,7 @@ class RegistrationStatistics:
hash_rate: float
difficulty: int
block_number: int
block_hash: bytes
block_hash: str
Copy link
Contributor

Choose a reason for hiding this comment

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

with going from bytes to str, lets make sure the E2E tests run and pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left the TODO

Copy link
Contributor

@ibraheem-opentensor ibraheem-opentensor left a comment

Choose a reason for hiding this comment

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

Nits - otherwise lgtm

@@ -73,7 +73,7 @@ def _get_real_torch():


def log_no_torch_error():
bittensor.btlogging.error(
bittensor.btlogging.logging.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Returns:
success (bool):
Flag is ``true`` if extrinsic was finalized or uncluded in the block. If we did not wait for finalization / inclusion, the response is ``true``.
success (bool): Flag is ``true`` if extrinsic was finalized or uncluded in the block. If we did not wait for finalization / inclusion, the response is ``true``.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the spelling uncluded needs to update in all doc strings

@@ -96,7 +95,7 @@ def prometheus_extrinsic(
bittensor.__console__.print(
f":white_heavy_check_mark: [green]Prometheus already Served[/green]\n"
f"[green not bold]- Status: [/green not bold] |"
f"[green not bold] ip: [/green not bold][white not bold]{net.int_to_ip(neuron.prometheus_info.ip)}[/white not bold] |"
f"[green not bold] ip: [/green not bold][white not bold]{net.ip_to_int(neuron.prometheus_info.ip)}[/white not bold] |"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this from int_to_ip to ip_to_int?

The console logs will be much easier to read if we dont have to convert it back. Otherwise we will just see a long number that doesnt mean anything unless you run it through a conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the result of a long refactoring) Thank you! I went for coffee

@RomanCh-OT RomanCh-OT force-pushed the async/roman/extrinsics-part3 branch from 6d4e808 to 01b82bd Compare June 10, 2024 23:40
@RomanCh-OT RomanCh-OT force-pushed the async/roman/extrinsics-part3 branch from 01b82bd to 6ac935b Compare June 10, 2024 23:42
@RomanCh-OT RomanCh-OT marked this pull request as ready for review June 10, 2024 23:58
@RomanCh-OT RomanCh-OT merged commit 64358c4 into merge-async Jun 10, 2024
4 of 7 checks passed
@RomanCh-OT RomanCh-OT deleted the async/roman/extrinsics-part3 branch June 10, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants