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

refactoring/roman/subtensor-part-2 #1913

Merged
merged 6 commits into from
May 22, 2024

Conversation

roman-opentensor
Copy link
Contributor

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

subtensor().validator_logits_divergence() last refactored method
Copy link
Contributor

@thewhaleking thewhaleking left a 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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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
Copy link
Contributor

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]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. thanks

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`.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good idea!

@opendansor opendansor marked this pull request as ready for review May 21, 2024 22:55
bittensor/subtensor.py Outdated Show resolved Hide resolved
bittensor/subtensor.py Outdated Show resolved Hide resolved
bittensor/subtensor.py Outdated Show resolved Hide resolved
Also, high-level refactoring (explicit places) was carried out for the entire module.

More detailed refactoring will continue with the "validator_logits_divergence" method.
@roman-opentensor roman-opentensor force-pushed the refactoring/roman/subtensor-part-2 branch from 9e9f3d3 to 540604e Compare May 22, 2024 00:03
@roman-opentensor roman-opentensor merged commit 721c1c8 into staging May 22, 2024
11 checks passed
@thewhaleking thewhaleking deleted the refactoring/roman/subtensor-part-2 branch May 22, 2024 21:01
roman-opentensor added a commit that referenced this pull request May 22, 2024
… it was decided to divide it into several stages.

This is part 2: current part affects everything up to the subtensor().tempo() method.
The "Hyper parameter calls" section was refactored by DRY.

Part 1 #1911
Part 2 #1913
roman-opentensor added a commit that referenced this pull request May 23, 2024
… it was decided to divide it into several stages.

This is part 2: current part affects everything up to the subtensor().tempo() method.
The "Hyper parameter calls" section was refactored by DRY.

Part 1 #1911
Part 2 #1913
roman-opentensor added a commit that referenced this pull request May 23, 2024
… it was decided to divide it into several stages.

This is part 4: current part affects everything up to the subtensor().tx_rate_limit() method.
Test coverage improved.

Part 1 #1911
Part 2 #1913
Part 3 #1923
@roman-opentensor roman-opentensor linked an issue May 24, 2024 that may be closed by this pull request
3 tasks
roman-opentensor added a commit that referenced this pull request May 24, 2024
… it was decided to divide it into several stages.

This is part 5: current part affects everything in this document except of renaming class `subtensor` according to the CamelCase rule.

Test coverage improved.

Part 1 #1911
Part 2 #1913
Part 3 #1923
Part 4 #1931
roman-opentensor added a commit that referenced this pull request May 24, 2024
… it was decided to divide it into several stages.

This is part 6: Renaming class 1 to comply with PEP8 and avoid namespace conflict. Refactor all references to the changed name.

Part 1 #1911
Part 2 #1913
Part 3 #1923
Part 4 #1931
Part 5 #1934
roman-opentensor added a commit that referenced this pull request May 24, 2024
… it was decided to divide it into several stages.

This is part 6: Renaming class 1 to comply with PEP8 and avoid namespace conflict. Refactor all references to the changed name.

Part 1 #1911
Part 2 #1913
Part 3 #1923
Part 4 #1931
Part 5 #1934
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.

bittensor.subtensor module refactoring
5 participants