bittensor

bittensor copied to clipboard

Bug

• You can pass any argument to code that uses 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 the required=True feature 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.
• When using strict=True, subparsers will wrongly complain about not recognizing arguments like --no_prompt

I'm not entirely sure what bt.config intends 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:

• 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.

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=True argument 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 --whatevercrapinsertedhere without 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.