-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cli: show debug options on --verbose #9693
Conversation
They are documented in wiki, https://github.com/iterative/dvc/wiki/Debugging,-Profiling-and-Benchmarking-DVC, although they may be hard to find.
It only works on macOS and Linux. On macOS, you have to press |
Thanks for the pointer! It probably makes sense to replace the blurb I wrote in the contributing docs with a reference to this wiki. I think having this info in contributing docs will make it a lot easier to find because the people who would need debugging tools (i.e. contributors) should be reading that. |
I updated the help strings based on info in the wiki, and I also modified the docs PR to point to the wiki. New
|
dvc/_debug.py
Outdated
parser.add_argument("--yappi", action="store_true", default=False, help=SUPPRESS) | ||
# For detailed info see: | ||
# https://github.com/iterative/dvc/wiki/Debugging,-Profiling-and-Benchmarking-DVC | ||
dvc_show_debug_options = os.environ.get("DVC_SHOW_DEBUG_OPTIONS", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit too verbose. Maybe better to use --debug
flag? Do you know maybe some examples of how other projects handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the long term, I'd really like to introduce the concept of "extensions", even if it's for our own use. We could enable extra functionalities if the devel
extension is enabled, for example, in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to opt for verbosity when I'm unsure. I think DVC_DEBUG=1 <cmd>
would likely work as well.
I don't see an easy way to add a --debug
option because the argparse object hasn't finished being constructed at the time where we need to determine if we use SUPRESS or add a help message. I think the environment variable approach is the cleanest way to allow these options to be conditionally exposed in --help
.
The other options I can think of are:
-
Add a
--debug
flag which prints a special help section, but that would need to be maintained separately, so that's undesirable. -
Check if
"--debug" in sys.argv
, but that's undesirable because 1. the parser might not be applied to sys.argv, and 2. there is a chance--debug
might appear on the command line, but not be intended as an argument flag (e.g.dvc add -- --debug
). Checkingsys.argv
won't respect the--
like argparse will. These issues probably won't arise in practice, and even if they did it would likely not matter because it just changes the content of the help message. Still, I think it is unclean design and the environment variable approach seems like the "idomatic" thing to do here. That being said,"--debug" in sys.argv
is simple, and I'm open to doing it this way. The only thing I think is important is that there is a way for a developer to unhide these options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skshetry Seems like a total overkill for this purpose. But I'm not even sure what extensions you mean, I suppose like git extensions? Don't quite see the need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Erotemic DVC_DEBUG sounds fine to me, let's not overthink it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at mercurial
, they show the options only when --verbose
is set including debugging related options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Erotemic regarding the cli option, there is a way to partially parse known options in argparse IIRC. Just checking argv (not sys.argv) is also fine. Mercurial's --verbose
sounds pretty good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to DVC_DEBUG and added it to env.
I do like the idea of showing the extra help with vebose
is set. At first I wasn't sure how to do it, but I think its possible as long as the parser is initialized with add_help=False
(which it is) and if we can ensure the verbose argument is added to the parser before we call add_debugging_flags
, which should just require reordering the code in get_parent_parser
.
Now that I see how to do this, I think it would be a good idea to remove the DVC_DEBUG environment variable entirely, and just add the extra help options is -v
is already given (which will make it much easier for a developer to stumble on this too, which is good).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the above change, I can revert if you want to go with the environ, but I think just respecting -v
is much cleaner now that I know about the add_help
argment to ArgumentParser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While playing with this I noticed two things and made changes accordingly. First the "cprofile-dump" option was not near "cprofile", so I moved it to keep similar options grouped. Secondly, if you run
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9693 +/- ##
==========================================
+ Coverage 90.53% 90.55% +0.02%
==========================================
Files 480 480
Lines 36382 36392 +10
Branches 5101 5228 +127
==========================================
+ Hits 32938 32955 +17
+ Misses 2852 2849 -3
+ Partials 592 588 -4
☔ View full report in Codecov by Sentry. |
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
line length linelen lint
Co-authored-by: skshetry <[email protected]>
Digging through the code I saw there were a lot of builtin tools for profiling / debugging, but (as far as I can tell) there was no mention of these in the docs, nor was there wany way for a user to become aware of them without digging into the code.
To remedy this, I added an environment variable
DVC_SHOW_DEBUG_OPTIONS
(which perhaps could be shorted toDVC_DEBUG
), and if this is specified the hidden CLI options will be shown to the user. Otherwise the behavior is the same.Also, when I tried to run a command with
--show-stack
I didn't see any difference in the output. So I'm not sure what it's actually doing.I've added a section in the contributing docs which mentions this new option. The corresponding PR is here: iterative/dvc.org#4662