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

Add commitment support to MockSubtensor #1635

Merged

Conversation

agoncharov-reef
Copy link
Contributor

This PR was intended to mock recently-added functions:

subtensor.commit(wallet, netuid, data)
subtensor.get_commitment(netuid, uid)
bittensor.extrinsics.serving.publish_metadata(wallet, netuid, type, data)
bittensor.extrinsics.serving.get_metadata(netuid, hotkey)

In order to "fairly" mock MockSubtensor.commit() and MockSubtensor.get_commitment(), we should mock MockSubtensor.substrate = MockSubstrate(). This way MockSubtensor.commit() will under the hood call bittensor.extrinsics.serving.publish_metadata() which will use our new MockSubstrate - so the behavior will be exactly the same as in production, except that substrate won't make an external call.

However, I see that MockSubtensor does not use any mocked substrate, thus it doesn't use "fair" low-level logic. This leads to following consequences:

  1. Low-level functions aren't mocked. One won't be able to test bittensor.extrinsics.serving.publish_metadata and bittensor.extrinsics.serving.get_metadata since there is only "production" substrate class and they use it.
  2. High-level MockSubtensor does not really use low-level functions, thus if some low-level function is created/modified/deleted, MockSubtensor will automatically diverge from original Subtensor.
  3. MockSubtensor inherits from subtensor, so if any new method is added to subtensor, then new non-mocked method appears in MockSubtensor.

Being said that, I would recommend to mock only real low-level network requests (the SubstrateInterface), so that everything else is mocked automatically now and in the future.

Anyway, taking into account existing codebase and approach, I added MockSubtensor.commit() and MockSubtensor.get_commitment() mocked methods.

Usage:

from bittensor.mock.subtensor_mock import MockSubtensor
from bittensor.wallet import wallet as Wallet

netuid = 9
payload = "some data"

wallet = Wallet(name='mockwall')
wallet = wallet.create_new_hotkey(overwrite=True, use_password=False, suppress=True)
wallet = wallet.create_new_coldkey(overwrite=True, use_password=False, suppress=True)

subtensor = MockSubtensor()
subtensor.create_subnet(netuid)
subtensor.force_register_neuron(netuid, wallet.hotkey.ss58_address, wallet.coldkey.ss58_address) 
subtensor.commit(wallet, netuid, payload)

uid = subtensor.get_uid_for_hotkey_on_subnet(
	hotkey_ss58=wallet.hotkey.ss58_address,
	netuid=netuid,
)
restored_payload = subtensor.get_commitment(netuid, uid)

assert restored_payload == payload

@ifrit98
Copy link
Contributor

ifrit98 commented Dec 27, 2023

Thanks for the PR, LGTM! Are there any associated tests we need to add for this change?

Using MockSubstrate is a good idea. We should incorporate this, however will change a lot of the bespoke existing test logic we have established using the current structure, but will be worth the effort. Openly welcome more PRs! (Added this to the backlog into a planned mocking/testing overhaul).

P.S. In the future, would you mind branching off from staging so we can streamline the inclusion process? Nice job!

@ifrit98 ifrit98 self-requested a review December 27, 2023 01:59
@ifrit98 ifrit98 changed the base branch from master to staging December 27, 2023 01:59
@ifrit98 ifrit98 merged commit 5093e5f into opentensor:staging Dec 27, 2023
9 checks passed
@agoncharov-reef
Copy link
Contributor Author

@ifrit98 Thank you! Agreed.
I will branch off from staging onwards, sorry for confusion.

@ifrit98 ifrit98 mentioned this pull request Jan 8, 2024
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.

None yet

2 participants