Feature/mvds00/test axon subprocess error checking #2226
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
added 2 commits
August 14, 2024 09:53
Issues in miner.py largely went unnoticed. This patch addresses this by checking whether the miner indeed runs, and at the end of the test terminating it and examining its output. It is asserted that the string "Exception" is not present in stdout/stderr. The subprocess is started with asyncio.create_subprocess_exec() so that it can be terminated easily. Added benefit is that escaping and quoting is not necessary anymore, as the arguments are passed as a list.
Bittensor uses nest_asyncio by default. This doesn't work well with uvicorn, that is used in the implementation of axon. By disabling nest_asyncio the miner now runs without errors.
heeres
force-pushed
the
feature/mvds00/test_axon-subprocess-error-checking
branch
from
40305f1
to
5070627
Compare
|
fixed ruff formatting issue |
thewhaleking
requested changes
Sep 3, 2024
| ) | ||
| logging.info("Neuron Alice is now mining") | ||
| await asyncio.sleep(1) # wait for the miner to start up or fail | ||
| if axon_process.returncode != None: |
| await asyncio.sleep( | ||
| 5 | ||
| ) # wait for 5 seconds for the metagraph to refresh with latest data | ||
| if axon_process.returncode != None: |
| stdout = textwrap.indent(stdout.decode("UTF8"), INDENT) | ||
| stderr = textwrap.indent(stderr.decode("UTF8"), INDENT) | ||
| assert ( | ||
| axon_process.returncode == None |
| stdout = textwrap.indent(stdout.decode("UTF8"), INDENT) | ||
| stderr = textwrap.indent(stderr.decode("UTF8"), INDENT) | ||
| assert ( | ||
| axon_process.returncode == None |
| @@ -109,4 +129,29 @@ async def test_axon(local_chain): | |||
| assert updated_axon.port == 8091 | |||
| assert updated_axon.hotkey == alice_keypair.ss58_address | |||
| assert updated_axon.coldkey == alice_keypair.ss58_address | |||
|
|
|||
| # Terminate miner and make sure nothing bad happened. | |||
| if axon_process.returncode == None: | |||
| try: | ||
| # try/except to prevent exception in case the process just ends now | ||
| axon_process.terminate() | ||
| except: |
There was a problem hiding this comment.
Let's not do bare excepts. Ideally, we should know what we're trying to catch, but if not, at least except Exception as e and log it.
| pass | ||
| await asyncio.sleep(1) | ||
|
|
||
| if axon_process.returncode == None: |
| try: | ||
| # try/except to prevent exception in case the process just ends now | ||
| axon_process.kill() | ||
| except: |
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
While working with the e2e tests, it appeared that the miner code did not function well, but the test never gave any indication of this. The two issues that this PR fixes are:
test_axon.pydoesn't check any output of the miner code.Although the miner code gets far enough to register an axon, making the test succeed, going forward it is not practical to not have everything working flawlessly under the hood.
Description of the Change
The subprocess to run a miner is now started with
asyncio.create_subprocess_execinstead ofasyncio.create_subprocess_shellso that it can be easily terminated by the test, to examine the output.The status of the subprocess is monitored. The NEST_ASYNCIO environment variable is set to 0 to disable nest_asyncio and fix the miner.
Alternate Designs
Rework of the test is actively worked on, this will be a separate PR on top of these commits.
Possible Drawbacks
Not known.
Verification Process
The test was run and with additional debugging it was observed that the miner now runs better.
Release Notes
N/A