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

[Fix] --help command #884

Merged
merged 8 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions bin/btcli
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
#!/usr/bin/env python3

import bittensor
import sys

import bittensor

if __name__ == '__main__':
bittensor.cli().run()
args = sys.argv[1:]
bittensor.cli(args=args).run()

# The MIT License (MIT)
# Copyright © 2021 Yuma Rao
# Copyright © 2022 Opentensor Foundation

# Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
# documentation files (the “Software”), to deal in the Software without restriction, including without limitation
Expand Down
4 changes: 2 additions & 2 deletions bittensor/_axon/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,13 @@ def add_args( cls, parser: argparse.ArgumentParser, prefix: str = None ):
def add_defaults(cls, defaults):
""" Adds parser defaults to object from enviroment variables.
"""
defaults.axon = bittensor.config()
defaults.axon = bittensor.Config()
camfairchild marked this conversation as resolved.
Show resolved Hide resolved
defaults.axon.port = os.getenv('BT_AXON_PORT') if os.getenv('BT_AXON_PORT') != None else 8091
defaults.axon.ip = os.getenv('BT_AXON_IP') if os.getenv('BT_AXON_IP') != None else '[::]'
defaults.axon.max_workers = os.getenv('BT_AXON_MAX_WORERS') if os.getenv('BT_AXON_MAX_WORERS') != None else 10
defaults.axon.maximum_concurrent_rpcs = os.getenv('BT_AXON_MAXIMUM_CONCURRENT_RPCS') if os.getenv('BT_AXON_MAXIMUM_CONCURRENT_RPCS') != None else 400

defaults.axon.priority = bittensor.config()
defaults.axon.priority = bittensor.Config()
defaults.axon.priority.max_workers = os.getenv('BT_AXON_PRIORITY_MAX_WORKERS') if os.getenv('BT_AXON_PRIORITY_MAX_WORKERS') != None else 10
defaults.axon.priority.maxsize = os.getenv('BT_AXON_PRIORITY_MAXSIZE') if os.getenv('BT_AXON_PRIORITY_MAXSIZE') != None else -1

Expand Down
24 changes: 15 additions & 9 deletions bittensor/_cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
# The MIT License (MIT)
# Copyright © 2021 Yuma Rao
# Copyright © 2022 Opentensor Foundation

# Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
# documentation files (the “Software”), to deal in the Software without restriction, including without limitation
Expand All @@ -21,41 +22,46 @@
import argparse
import os
import sys
from typing import List
from typing import List, Optional

import bittensor
import torch
from rich.prompt import Confirm, Prompt
from substrateinterface.utils.ss58 import ss58_decode, ss58_encode

from . import cli_impl

console = bittensor.__console__

class cli:
"""
Create and init the CLI class, which handles the coldkey, hotkey and tau transfer
Create and init the CLI class, which handles the coldkey, hotkey and tao transfer
"""
def __new__(
cls,
config: 'bittensor.Config' = None,
cls,
config: Optional['bittensor.Config'] = None,
args: Optional[List[str]] = None,
) -> 'bittensor.CLI':
r""" Creates a new bittensor.cli from passed arguments.
Args:
config (:obj:`bittensor.Config`, `optional`):
bittensor.cli.config()
args (`List[str]`, `optional`):
The arguments to parse from the command line.
"""
if config == None:
config = cli.config()
config = cli.config(args)
cli.check_config( config )
return cli_impl.CLI( config = config)

@staticmethod
def config() -> 'bittensor.config':
def config(args: List[str]) -> 'bittensor.config':
""" From the argument parser, add config to bittensor.executor and local config
Return: bittensor.config object
"""
parser = argparse.ArgumentParser(description="Bittensor cli", usage="btcli <command> <command args>", add_help=True)
parser = argparse.ArgumentParser(
description=f"bittensor cli v{bittensor.__version__}",
usage="btcli <command> <command args>",
add_help=True)

cmd_parsers = parser.add_subparsers(dest='command')
overview_parser = cmd_parsers.add_parser(
Expand Down Expand Up @@ -611,7 +617,7 @@ def config() -> 'bittensor.config':
required=False
)

return bittensor.config( parser )
return bittensor.config( parser, args=args )

@staticmethod
def check_config (config: 'bittensor.Config'):
Expand Down
47 changes: 39 additions & 8 deletions bittensor/_config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
# The MIT License (MIT)
# Copyright © 2021 Yuma Rao
# Copyright © 2022 Opentensor Foundation

# Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
# documentation files (the “Software”), to deal in the Software without restriction, including without limitation
Expand All @@ -19,12 +20,14 @@
# DEALINGS IN THE SOFTWARE.

import os
from argparse import ArgumentParser
import sys
from argparse import ArgumentParser, Namespace
from typing import List, Optional

import bittensor
import yaml
from loguru import logger

import bittensor
from . import config_impl

logger = logger.opt(colors=True)
Expand All @@ -37,13 +40,15 @@ class InvalidConfigFile(Exception):
""" In place of YAMLError
"""

def __new__( cls, parser: ArgumentParser = None, strict: bool = False ):
def __new__( cls, parser: ArgumentParser = None, strict: bool = False, args: Optional[List[str]] = None ):
Copy link
Contributor

@Eugene-hu Eugene-hu Aug 22, 2022

Choose a reason for hiding this comment

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

Is there a reason for this change? I will be hesitant to change the base config class unless necessary. Every single bittensor object initiates a base config object. So ideally, we should keep it as lightweight as possible. The config class is just a whole bag of worms XD

Copy link
Contributor

@Eugene-hu Eugene-hu Aug 22, 2022

Choose a reason for hiding this comment

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

I also think the config class is in need of a cleanup, so it might be good for the next update.

Copy link
Collaborator Author

@camfairchild camfairchild Aug 23, 2022

Choose a reason for hiding this comment

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

this is an inversion of control of the sys.argv from the command line.

Normally argparser gets it internally, if you don't specify which args to parse, by calling sys.argv the same way I added in the neuron __main__

I added it as an optional arg and lower down it defaults to calling sys.argv if None, so the class will function as normal without the arg specified.

The IoC is important for testing if btcli --help works correctly and has the text we expect. Otherwise I would have to patch argparser, which is subject to API changes. This is recommended from some SO posts I read.

Edited: dep injection -> IoC

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this sounds like a temporary change before the cleanup, i would generate some issues in this repository to not forget about the tech debt we have around this code.

r""" Translates the passed parser into a nested Bittensor config.
Args:
parser (argparse.Parser):
Command line parser object.
strict (bool):
If true, the command line arguments are strictly parsed.
args (list of str):
Command line arguments.
Returns:
config (bittensor.Config):
Nested config object created from parser arguments.
Expand All @@ -69,8 +74,15 @@ def __new__( cls, parser: ArgumentParser = None, strict: bool = False ):
except Exception as e:
config_file_path = None

# Get args from argv if not passed in.
if args == None:
args = sys.argv[1:]

# Parse args not strict
params = cls.__parse_args__(args=args, parser=parser, strict=False)

# 2. Optionally check for --strict, if stict we will parse the args strictly.
strict = parser.parse_known_args()[0].strict
strict = params.strict

if config_file_path != None:
config_file_path = os.path.expanduser(config_file_path)
Expand All @@ -83,10 +95,8 @@ def __new__( cls, parser: ArgumentParser = None, strict: bool = False ):
print('Error in loading: {} using default parser settings'.format(e))

# 2. Continue with loading in params.
if not strict:
params = parser.parse_known_args()[0]
else:
params = parser.parse_args()
params = cls.__parse_args__(args=args, parser=parser, strict=strict)

_config = config_impl.Config()

# Splits params on dot syntax i.e neuron.axon_port
Expand All @@ -107,6 +117,27 @@ def __new__( cls, parser: ArgumentParser = None, strict: bool = False ):

return _config

@staticmethod
def __parse_args__( args: List[str], parser: ArgumentParser = None, strict: bool = False) -> Namespace:
"""Parses the passed args use the passed parser.
Args:
args (List[str]):
List of arguments to parse.
parser (argparse.ArgumentParser):
Command line parser object.
strict (bool):
If true, the command line arguments are strictly parsed.
Returns:
Namespace:
Namespace object created from parser arguments.
"""
if not strict:
params = parser.parse_known_args(args=args)[0]
else:
params = parser.parse_args(args=args)

return params

@staticmethod
def full():
""" From the parser, add arguments to multiple bittensor sub-modules
Expand Down
32 changes: 29 additions & 3 deletions tests/integration_tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# The MIT License (MIT)
# Copyright © 2022 Yuma Rao
# Copyright © 2022 Opentensor Foundation

# Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
# documentation files (the “Software”), to deal in the Software without restriction, including without limitation
Expand All @@ -16,18 +17,16 @@
# DEALINGS IN THE SOFTWARE.


import sys
import unittest
from types import SimpleNamespace
from typing import Dict
from unittest.mock import ANY, MagicMock, call, patch
import pytest

import bittensor
from bittensor._subtensor.subtensor_mock import mock_subtensor
from bittensor.utils.balance import Balance
from substrateinterface.base import Keypair
from substrateinterface.exceptions import SubstrateRequestException

from tests.helpers import CLOSE_IN_VALUE


Expand Down Expand Up @@ -1327,6 +1326,33 @@ def test_list_no_wallet( self ):
# This shouldn't raise an error anymore
cli.run()

def test_btcli_help():
"""
Verify the correct help text is output when the --help flag is passed
"""
with pytest.raises(SystemExit) as pytest_wrapped_e:
with patch('argparse.ArgumentParser._print_message', return_value=None) as mock_print_message:
args = [
'--help'
]
bittensor.cli(args=args).run()

# Should try to print help
mock_print_message.assert_called_once()

call_args = mock_print_message.call_args
args, _ = call_args
help_out = args[0]

# Expected help output even if parser isn't working well
## py3.6-3.9 or py3.10+
assert 'optional arguments' in help_out or 'options' in help_out
# Expected help output if all commands are listed
assert 'positional arguments' in help_out
# Verify that cli is printing the help message for
assert 'overview' in help_out
assert 'run' in help_out


if __name__ == "__main__":
cli = TestCli()
Expand Down