-
Notifications
You must be signed in to change notification settings - Fork 302
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
refactoring/roman/subtensor-part-2 #1913
Conversation
subtensor().validator_logits_divergence() last refactored method
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.
Will continue looking at this in the morning.
error (str): Error message if transfer failed. | ||
""" | ||
|
||
@retry(delay=2, tries=3, backoff=2, max_delay=4, logger=_logger) | ||
@retry(delay=1, tries=3, backoff=2, max_delay=4, logger=_logger) |
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.
Why the change to delay?
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.
Changing the delay from 2 to 1 will reduce the initial delay before the first retry and generally shorten the time between retries. This will speed up the retry process in case of errors.
When I tested changing the delay to 1 second, there were no errors, but the command execution time was reduced.
bittensor/subtensor.py
Outdated
@@ -1431,7 +1426,8 @@ def get_existential_deposit(self, block: Optional[int] = None) -> Optional[Balan | |||
balances below this threshold can be reaped to conserve network resources. | |||
|
|||
Args: | |||
block (Optional[int], optional): Block number at which to query the deposit amount. If ``None``, the current block is used. | |||
block (Optional[int], optional): Block number at which to query the deposit amount. If ``None``, the |
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.
Do we need , optional
if we already have Optional[int]
?
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.
good point. thanks
bittensor/subtensor.py
Outdated
placeholder1 (int, optional): Placeholder parameter for future extensions. Default is `0`. | ||
placeholder2 (int, optional): Placeholder parameter for future extensions. Default is `0`. | ||
wait_for_inclusion (bool, optional): Waits for the transaction to be included in a block. Default is | ||
`False`. |
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.
Maybe use the double backticks
for these docstring annotations, to keep consistency.
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.
yeah, good idea!
subtensor().validator_logits_divergence() last refactored method
Also, high-level refactoring (explicit places) was carried out for the entire module. More detailed refactoring will continue with the "validator_logits_divergence" method.
9e9f3d3
to
540604e
Compare
Since the refactoring of this module would turn into a large project, it was decided to divide it into several stages.
This is part 2: current part affects everything up to the subtensor().validator_logits_divergence() method.
Part 1 #1911