Skip to content
This repository has been archived by the owner on Feb 17, 2022. It is now read-only.

New timeit feature #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

eckpang
Copy link

@eckpang eckpang commented Mar 3, 2014

Added a new timeit feature to measure the execution time of a command.

Added a new extra_options parameter to the Run command. The timeit feature is only available if extra_options is set to true. extra_options is False by default.

An example and explanation of the timeit feature can be found in the example_extra.py file

I chose to hide the timeit option by default to avoid clouding the real command options list for users who don't need it. In the future other extra options can be added - some ideas include repeating the command x number of times to do benchmarking, and do a random sleep before running a command to avoid server cron jobs from running the command at the exact same time.

I welcome feedback on whether these features are useful, and whether there are some better ways to organize them. Thanks

…mand.

- Minor improvement of an error message.
@@ -122,12 +122,19 @@
# main:
# If set, Commandr will use the supplied value as the command name to run
# if no command name is supplied. It will override any previous values.
#
# extra_options:

Choose a reason for hiding this comment

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

Instead of "extra_options", I think "instrumentation_options" is a better name for the feature.

@kevinballard
Copy link

I'm on the fence as to whether this is a good idea to include. Generally, I prefer to keep tools as simple as possible. For commandr that means that any functionality that could just as easily be implemented in the wrapped functions themselves (or as a separate decorator) should go there, and not hidden in commandr's internals.

That said, this is certainly related to command execution, and would be a bit tricky to implement cleanly.

@eckpang
Copy link
Author

eckpang commented Mar 3, 2014

Thanks for the feedback. I agree with all the coding style comments.

I think the name instrumentation_options is good. The reason I called it extra_options is that I was anticipating other types of "hidden" options in the future if this change is accepted :)

Regarding the last comment - I think the advantage of building the timeit functionality as an option is that the instrumentation can be switched on/off at run-time (once instrumentation_option is enabled in the run function). If the functionality is implemented inside the code as wrapped functions or decorator, the code needs to change to switch on instrumentation. The code change might be trivial, but if the script is in a production environment and one wants to do some quick benchmarking, one may not want to make temporary changes to a script.
I was also considering not hiding this option, but instead exposing this option for ALL commands, but in the help clearly show that these are built-in instrumentation options with no shortcuts.

@kevinballard
Copy link

I don't think this functionality should be on by default -- instrumentation is useful for a subset of applications, but most will never make use of them. Also, the 'timeit' module has side-effects (like disabling the GC) that you really don't want to be generally enabled.

I think we will be shortly adding a plugin style system to extend commandr without having to be integrated directly into the core. I think this would be a good use case for that system.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants