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

Moved load_state() to the neuron.__init__ #55

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

the-mx
Copy link
Contributor

@the-mx the-mx commented Jan 24, 2024

In the current code, the template.base.validator calls self.sync during initialization which leads to the state being saved (calling self.save_state). And it happens before state being loaded in the neuron.validator initialization. Since the sync function is in the neuron code and it calls save_state there, it's logical to move load_state to neuron initialization. An alternative would be not to call sync during initialization, since it's being called the first thing in the run method.

@ifrit98
Copy link
Contributor

ifrit98 commented Feb 7, 2024

Your reasoning is sound, however the miner typically does not have state to load or save. So if this is to be accepted, it would require an implementation of load_state() on the miner side that would simply return without the warning in the base class.

@ifrit98 ifrit98 self-requested a review February 7, 2024 16:00
@@ -99,6 +99,8 @@ def __init__(self, config=None):
)
self.step = 0

self.load_state()
Copy link
Contributor

Choose a reason for hiding this comment

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

Miner needs its own null definition.

@gus-opentensor gus-opentensor added the help wanted Extra attention is needed label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants