Tests for metagraph, highlighting bug #2203
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@ibraheem-opentensor could you review when you have time please |
|
Hey @mvds00 |
|
I will submit a PR. |
heeres
force-pushed
the
feature/mvds00/additional-tests-for-metagraph-highlighting-bug
branch
from
907f467 to
abbe79a
Compare
|
I rebased the PR on staging, the changes are still relevant. It tests save/load - I saw another PR was posted already fixing that. |
thewhaleking
previously approved these changes
Sep 3, 2024
added 4 commits
September 3, 2024 13:42
The following does not do what the author probably thought:
assert 'a' and 'b' in s
as it evaluates as:
assert 'a' and ('b' in s)
and hence:
assert 'b' in s
This is fixed by splitting the assertions in two or by changing the
condition to ('a' in s and 'b' in s).
Using variable netuid instead of magic constant 1 (which happens to be the netuid of the added subnet) provides context to the usage of the value 1 and makes the test more future proof.
Saving and loading should transfer the complete state of the metagraph.
A minor tweak to the test that increases coverage of the test.
heeres
force-pushed
the
feature/mvds00/additional-tests-for-metagraph-highlighting-bug
branch
from
abbe79a to
b63fae3
Compare
|
I just rebased it on staging, some tests fail but that is on purpose IIRC. |
ibraheem-opentensor
force-pushed
the
staging
branch
from
f13f5f3 to
73d9fd4
Compare
mvds00
dismissed
thewhaleking’s stale review
The merge-base changed after approval.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
The e2e test that tests metagraph has some flaws. Some assertions don't work as intended and a bug in metagraph is not caught. While I was at it I fixed some low hanging fruit, that is, some fragile constructions that may turn into bugs later on.
Description of the Change
assert 'a' and 'b' in sas it doesn't test whether both'a'and'b'are ins1withnetuid.save()and.load()on metagraphnote that the last test fails, but that is due to a bug in bittensor that has to be solved. This PR demonstrates there is a bug, and provides a test to see whether a (to be written) fix solves the bug.
Alternate Designs
Not relevant.
Possible Drawbacks
N/A
Verification Process
By doing
metagraph_pre_dave = subtensor.metagraph(netuid=netuid)instead ofmetagraph_pre_dave.load()it was verified that the assertions comparing distinct metagraph objects should work, if the metagraphs are in sync.Release Notes
N/A