btcli/bt.config: various improvements #2223
Conversation
heeres
force-pushed
the
feature/mvds00/improve-btcli-bt-config
branch
4 times, most recently
from
4be7aa7
to
a87e7f8
Compare
bittensor/config.py
Outdated
| config.apply_to_parser_recursive(cmd_parser, callback, depth=depth + 1) | ||
|
|
||
| @staticmethod | ||
| def add_standard_args(parser): |
There was a problem hiding this comment.
To keep a consistent naming of adding arguments across the entire bittensor, pls renameadd_standard_args to add_args.
| cmd_parser._defaults.clear() # Needed for quirk of argparse | ||
|
|
||
| def l_set_defaults(l_parser): | ||
| ## Set the defaults to argparse.SUPPRESS, should remove them from the namespace |
There was a problem hiding this comment.
Convert this to a dostring. Also, please make the function name l_set_defaults a little more descriptive. Example: load_set_defaults.
Also, add type annotation.
bittensor/config.py
Outdated
| @@ -231,6 +172,63 @@ def __init__( | |||
| ] | |||
| } | |||
|
|
|||
| @staticmethod | |||
| def apply_to_parser_recursive(parser, callback, depth=0): | |||
There was a problem hiding this comment.
Pls add single line docstring
heeres
force-pushed
the
feature/mvds00/improve-btcli-bt-config
branch
from
a87e7f8
to
f740b23
Compare
|
Fixed first round of comments, for now as a separate commit. Also fixed two issues that came up in CI, as we are now seeing previously buried issues pop up due to strict checking. Some unknown |
Nice. Could you pls fix ruff. I would like to see that all checkers passed. |
|
So this is definitely an improvement, but config and flags in general are changed drastically with the new btcli (soon to release fully): https://github.com/opentensor/btcli |
- implement recursive apply function for (sub^n)argparser - use recursive apply to eliminate repeated code - move adding standard bt.config args to separate function - *use recursive apply to add standard bt.config args to all parsers - *make strict=True the default - eliminate redundant COMMANDS.item.name field - remove obsolete COMMANDS looping code - remove redundant dest= argument - fix some comments The changes marked * implement a functional improvement, the rest is for improved maintainability.
…ument 'hyperparameters'
…ment '--proposal_hash'
…from test By using patch() to mock bittensor.wallet, the mechanics of bittensor.wallet are skipped, including adding the args for parser. They have no effect and are not recognized, leading to an error.
heeres
force-pushed
the
feature/mvds00/improve-btcli-bt-config
branch
from
e5bd729
to
d7cb599
Compare
ibraheem-opentensor
force-pushed
the
staging
branch
from
f13f5f3
to
73d9fd4
Compare
Bug
bt.config()with default kwargs, and it will not complain. This leads to confusion on all levels (developers, devops, users).bt.config()does some special magic that kills therequired=Truefeature of argparse; it will complain that the required argument is not set, even when it is set. This leads to developers implementing workarounds such as setting semi-random default values and/or additional post-argparse checks that could/should already be done in argparse. This bug seems to originate in the recursive nature of argparse as used in bittensor.strict=True, subparsers will wrongly complain about not recognizing arguments like--no_promptI'm not entirely sure what
bt.configintends to do at every turn, but I think the changes described below don't make it worse.Description of the Change
While fixing the bugs described above, various other small (cosmetic) improvements were done. Taken from the commit message:
The changes marked * implement a functional improvement, the rest is for improved maintainability.
Alternate Designs
Rework
bt.config, as it is not clear what it intends to add to argparse, that already supports tons of features.Possible Drawbacks
People may experience code not starting anymore because they supply unknown arguments that now lead to errors due to strict checking. This is intended. Having non-functional arguments is not useful and confusing.
Verification Process
I tested this on some bittensor code I'm writing right now, by adding a
required=Trueargument and observing that the argument is now required and that the code doesn't bail out on a supposedly missing argument. I tested to see that I now cannot add--whatevercrapinsertedherewithout argparse complaining about an unknown arg.The CI checks further serve as a broad verification of the argparse mechanics.
Release Notes
Any invalid arguments will now lead to an error, instead of being silently ignored, or treated otherwise depending on your exact implementation. In case you encounter an invalid argument error, as a first step remove the offending argument(s) and see what happens.