-
Notifications
You must be signed in to change notification settings - Fork 131
ENH: read TOML files for configurations #335
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 64.84% 68.46% +3.62%
==========================================
Files 13 15 +2
Lines 1041 1243 +202
Branches 228 277 +49
==========================================
+ Hits 675 851 +176
- Misses 306 326 +20
- Partials 60 66 +6
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Getting a start on this review. There are a lot of changes, so I'm going to need to do a few passes over it before we merge, but at a high level it looks very good. In my first pass, I see a few things:
I'm also curious, what was the LLM usage on this PR like? I'm wondering if the minor changes came from a LLM editor, or if you were explicitly making the style consistent. |
Good point, will do. So basically instead of
Yeah this could use some deliberation... on the one hand, it's very convenient to just have to specify all the config in a central TOML file (including our defaults) and read from it, but on the other it does introduce a dependency. Maybe just for the sake of keeping the requirement list short, it is worth it to have a bit of duplication and put the default configs also in the Python code... it's a tradeoff that we have to decide on. (One concern though is that that may cause the behaviors between platforms to go out of sync, if a future contributor (could easily be us in a month) only updated the code-side configs and forgot to do so to the rc-side configs. But since our dev machines all have newer Python versions I guess we can put a
I guess you mean when we call that to get the
That's a brilliant idea! Yep, we're currently missing the option to ignore config files, and that'd be helpful.
I see your point. In hindsight I was perhaps overzealous in making those changes... the intent was to make them backwards compatible with vanilla
Never really used one – I'm staunchly a |
Yes: On tomli: I'm fine with keeping it as is. It's so small and standard, and modern Python versions don't need it. On @functools.lru_cache-ed function: That works for me, and is how I've handled it in the past. I try to make imports happen as soon as possible. Nothing is worse than a bottleneck in response time when you startup python (e.g. torch, pytorch-lightning, etc...). The option to ignore configs is critical for testing and debugging. xdoctest: it's fine to leave it. The more compatible the better I suppose. All doctest code should be forward compatible with xdoctest. Using xdoctest as a runner isn't supposed to be "better", it just makes it faster for me to write - although I suppose it would be possible to write a "normalizer" in xdoctest so it can factor itself out if needed. LLM: I'm gvim+terminal as well (gvim users exist), but I have been using the free LLMs like DeepSeek, ChatGPT, and a few ones I can run locally. They save a ton of time. It would be interesting to see if you gave it a default config and asked it to generate documentation (maybe provide a diff of the other code for context, so it can figure out what they do), and see how that compares to the docs you wrote. First thing I did for this is throw the diff at an LLM and ask it for places to look into first. It was also used for finding things (like checking if you did implement the null config case, or if it just wasn't documented). All my usage is copy+paste, but I've also been considering a switch to vscode to get copilot. If you have a GPU, open-webui in a docker container is a nice way to use them. I think nvim might have good integrations, but vim/gvim do seem to be lacking. |
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.
Partial review while I had a spare cycle.
kernprof.py
Outdated
|
||
Run and profile a python script. | ||
Run and profile a python script or module.Boolean options can be negated by passing the corresponding flag (e.g. `--no-view` for `--view`). |
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.
Run and profile a python script or module.Boolean options can be negated by passing the corresponding flag (e.g. `--no-view` for `--view`). | |
Run and profile a python script or module. |
The extra docs "Boolean options can be negated by passing the corresponding flag (e.g. --no-view
for --view
" should probably be less prominent in some notes section if we want it at all.
What do you think about instead of using the --flag
--no-flag
convention, using --flag
always results in setting the option to True, but if you want to explicitly set it you use a key/value pair --flag=True
or --flag=False
.
Another one of my design soapboxes is that you shouldn't have to delete a flag to disable it, you should always be able to set state via key/value pairs on the command line. However, I didn't add that feature here as my primary goal with this package is maintenance and facilitating development from other devs like yourself, as I spend quite a bit of effort developing other packages.
One of those packages is scriptconfig. I don't want to add a dependency on it here, but we could borrow some code from it. Namely: BooleanFlagOrKeyValAction, which allows for flexible specification of boolean variables, e.g.:
--flag > {'flag': True}
--flag=1 > {'flag': True}
--flag True > {'flag': True}
--flag True > {'flag': True}
--flag False > {'flag': False}
--flag 0 > {'flag': False}
--no-flag > {'flag': False}
--no-flag=0 > {'flag': True}
--no-flag=1 > {'flag': False}
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.
Agreed, big fan of this style of boolean flags (I think I did the same for pytest-autoprofile
); will implement.
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.
Just realized that there will be unintended consequences: short flags can no longer be concatenated since they are no longer simple boolean flags. But that can be circumvented by creating separate actions for the short and long flags I guess.
kernprof.py
Outdated
# Make sure the script's directory is on sys.path instead of | ||
# just kernprof.py's. | ||
sys.path.insert(0, os.path.dirname(script_file)) | ||
__file__ = script_file | ||
__name__ = '__main__' | ||
|
||
if options.output_interval: | ||
# XXX: why are we doing this here (5a38626) and again below? |
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 think we can just remove one of these. I noticed this too, and I think it was a mistake.
config options are loaded | ||
""" | ||
conf, source = get_config(*args, **kwargs) | ||
cli_conf = {key.replace('-', '_'): value |
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've also tackled this in scriptconfig, although the code for doing so was complicated by internal changes to argparse in python/cpython@c02b7ae
Code is here:
https://gitlab.kitware.com/utils/scriptconfig/-/blob/main/scriptconfig/argparse_ext.py?ref_type=heads#L423
Might not be worth the extra complexity, but I figured I would point it out.
736db2a
to
7c70075
Compare
line_profiler/line_profiler_rc.toml Default configuration file line_profiler/toml_config.py[i] New module for reading TOML config files
MANIFEST.in, setup.py Configured to include `line_profiler/line_profiler_rc.toml` in source and wheel distributions requirements/runtime.txt Added dependency `tomli` for Python < 3.11 (to stand in for `tomllib`)
kernprof.py - Made all `line_profiler` imports unconditional (the ship has sailed, there's already an unconditional import for `line_profiler.profiler_mixin.ByCountProfilerMixin`) - For each boolean option (e.g. `--view`): - Added a corresponding negative option (e.g. `--no-view`) - Changed the default value from `False` to `None`, so that we can distinguish between cases where the negative option is passed and no option is passed (and in that case read from the config (TODO)) main(), find_module_script(), find_script() Added argument `exit_on_error` to optionally prevent parsing errors from killing the interpretor
kernprof.py __doc__ Updated with newest `kernprof --help` output short_string_path() New helper function for abbreviating paths _python_command() - Replaced string comparison between paths with `os.path.samefile()` - Updated to use abbreviated paths where possible main() - Updated description to include documentation for the negative options - Added option `--config` for loading config from a specific file instead of going through lookup - Updated `const` value for the bare `-i`/`--output-intereval` option (the old value 0, equivalent to not specifying the option, doesn't really make sense) - Grouped options into argument groups for better organization - Updated help texts for options to be more stylistically consistent and to show the default values - Updated help texts for the `-p`/`--prof-mod` option to show an example - Updated help texts for the `--prof-imports` to be more in line with what it actually does (see docstring of `line_profiler.autoprofile.ast_tree_profiler.AstTreeProfiler`) - Added option resolution: values of un-specified flags now taken from the specified/looked-up config file
line_profiler/toml_config.py[i] find_and_read_config_file() New argument `env_var` for controlling whether/whence to read the config from the environment if not specified via `config` get_config() New arguemnt `read_env` for controlling whether to read the config from the environment variable `${LINE_PROFILER_RC}` if specified via `config`
kernprof.py Moved common utilities to `line_profiler/cli_utils.py` line_profiler/cli_utils.py[i] New module for common utilities shared by `kernprof` and `python -m line_profiler`
kernprof.py::main() Now passing the received `-c`/`--config` onto `LineProfiler.print_stats()` line_profiler/line_profiler.py[i] <Overall> Formatting changes (avoid using hanging indents where suitable, to be more consistent with the rest of the codebase) LineProfiler.print_stats(), show_func(), show_text() - Added optional argument `config` to allow for specifying the config file - Now reading output column widths from the `tool.line_profiler.show.column_widths` table in config files main() - Refactored to use the `.cli_utils` - Added description for the CLI application - Added negative options to the boolean options - Added option `-c`/`--config` to read default values for the options from the `tool.line_profiler.cli` table line_profiler/line_profiler_rc.toml - Added documentation for the environment variable `${LINE_PROFILER_RC}` - Added subtables `tool.line_profiler.cli` and `tool.line_profiler.show.column_widths`
line_profiler.toml_config.py[i] get_subtable() - Added doctest - Added type check that the returned object is a mapping get_headers() New function for getting the subtable headers from a table get_config() - Updated docs with reference to the new `tool.line_profiler.cli` and `.show.column_widths` tables - Added check for subtable existence - Fixed traceback and error message when the table is malformed
kernprof.py::_python_command Migrated definition to `line_profiler/cli_utils.py::get_python_executable()` line_profiler/cli_utils.py::get_python_executable() New function used by both `kernprof` and `line_profiler.explicit_profiler`
line_profiler/explicit_profiler.py GlobalProfiler __doc__ Updated __init__() - Added argument `config` to allow for explicitly providing a config file - Now reading `.{setup,write,show}_config` and `.output_prefix` from the config file, instead of using hard-coded values show() Minor refatoring and reformatting _python_command() Now using `.cli_utils.get_python_executable()` line_profiler/line_profiler_rc.toml::[tool.line_profiler.write] Added item `output_prefix`, corresponding to `line_profiler.explicit_profiler.GlobalProfiler.output_prefix`
line_profiler/toml_config.py::get_config() - Now promoting `config` to `pathlib.Path` objects early so as to catch bad arg types - Now raising `ValueError` or `FileNotFoundError` if a `config` is specified and loading fails tests/test_toml_config.py New test module for tests related to `line_profiler.toml_config`: - test_environment_isolation() Test that the fixture we use suffices to isolate the tests from the environment - test_default_config_deep_copy() Test that `get_default_config()` returns fresh, deep copies - test_table_normalization() Test that `get_config()` always normalizes the config entires to supply missing entires and remove spurious ones - test_malformed_table() Test that we get a `ValueError` from malformed TOML files - test_config_lookup_hierarchy() Test the hierarchy according to which we resolve which TOML to read the configs from
tests/test_explicit_profile.py test_*() Updated to use `tempfile.TemporaryDirectory()` instead of `tempfile.mkdtemp()` so that the tmpdirs are cleaned up regardless of the test outcome test_explicit_profile_with_customized_config() New test for customizing explicit profiling with a user-supplied TOML config file
tests/test_autoprofile.py test_*() Updated to use `tempfile.TemporaryDirectory()` instead of `tempfile.mkdtemp()` so that the tmpdirs are cleaned up regardless of the test outcome test_autoprofile_with_customized_config() New test for customizing `kernprof` auto-profiling and `python -m line_profiler` output formatting with a user-supplied TOML config file
line_profiler/explicit_profiler.py::GlobalProfiler Updated docstring line_profiler/toml_config.py::targets Changed lookup target from `line_profiler_rc.toml` -> `line_profiler.toml` tests/test_toml_config.py::test_config_lookup_hierarchy() Updated test to use `line_profiler.toml` instead of `line_profiler_rc.toml`
line_profiler/line_profiler.py[i] minimum_column_widths Removed global constant get_minimum_column_widths() New cached callable for getting the above value
kernprof.py __doc__ Updated with new `kernprof --help` output main() Added a `--no-config` flag for disabling the loading of non-default configs line_profiler/cli_utils.py[i]::get_cli_config() - Added the explicit named argument `config` - Added processing for boolean values of `config` (true -> default behavior, false -> fall back for `get_default_config()`) line_profiler/line_profiler.py[i] LineProfiler.print_stats(), show_func(), show_text() Added handling for boolean values of `config` main() Added a `--no-config` flag for disabling the loading of non-default configs tests/test_autoprofile.py test_autoprofile_with_customized_config() Fixed malformed indentation test_autoprofile_with_no_config() New test for disabling config lookup
line_profiler/cli_utils.py[i]::get_cli_config() Rolled back last commit line_profiler/explicit_profiler.py[i]::GlobalProfiler - Updated docstring - Added missing `config` argument to `.__init__()` in the stub file line_profiler/line_profiler.py Removed wrapper function around `line_profiler.toml_config.get_config()` line_profiler/toml_config.py[i]::get_config() Added handling for `config = <bool>`: - `False`: don't look up or load any user-supplied config and just use the default - `True`: same as `None` (default behavior) tests/test_toml_config.py::test_config_lookup_hierarchy() Now also testing `get_config(True)` and `get_config(False)`
line_profiler/cli_utils.py[i] <General> Updated docstring to use double backticks for inlined code to be friendlier towards sphinx/RST add_argument() - Updated call signature: - `parser_like` now positional-only - Added `arg` between `parser_like` and `*args` to indicate that at least one positional argument should be passed to `parser_like.add_argument()` - Now adding separate actions for long and short boolean flags so that the long option can be specified in both no-arg and single-arg forms - Now skipping the generation of the complementary falsy action instead of raising an `AssertionError` when the boolean flag doesn't have a long form boolean() New function for parsing strings into booleans
line_profiler/explicit_profiler.py::GlobalProfiler._implicit_setup() Updated parsing of environment variables to use `line_profiler.cli_utils.boolean()` line_profiler/line_profiler.py::main() - Removed note on boolean options in the parser description - Removed parenthetical remark "boolean option" from option help texts
kernprof.py __doc__ Updated with the latest `kernprof --help` output, and with a narrower window so that the lines aren't too long __doc__ RepeatedTimer.__doc__ Added linter-friendly `noqa` comment for long lines in docstrings meant for `sphinx` consumption RepeatedTimer.start() main() Wrapped certain long lines of code main() - Removed note on boolean options in parser description - Removed parenthetical remark "boolean option" in option help texts - Removed redundant instantiation of `RepeatedTimer` - Refactored chained if-elif-else when executing code in non-autoprofiling mode
tests/test_cli.py parser() New fixture (example parser) test_boolean_argument_help_text() New test for the help texts for boolean options generated by `line_profiler.cli_utils.add_argument()` test_boolean_argument_parsing() New test for the parsing of long and short boolean options
Synopsis
This PR adds the capability to configure
kernprof
andline_profiler
via TOML files likepyproject.toml
.Namespaces
[tool.line_profiler.kernprof]
:Defaults for command-line options of
kernprof
[tool.line_profiler.cli]
:Defaults for command-line options of
python -m line_profiler
[tool.line_profiler.setup]
:Defaults for the
line_profiler.profile.setup_config
table[tool.line_profiler.write]
:Defaults for the
line_profiler.profile.write_config
table, except for theoutput_prefix
key, which maps instead toline_profiler.profile.output_prefix
[tool.line_profiler.show]
:Defaults for the
line_profiler.profile.show_config
table, except for the subtable[tool.line_profiler.show.column_widths]
(see below)[tool.line_profiler.show.column_widths]
:Default column widths for
line_profiler.LineProfiler.print_stats()
,line_profiler.line_profiler.show_text()
, etc.Specification and discovery
The TOML config file in question can be specified by
--config
flag (or-c
forpython -m line_profiler
) to thekernprof
andpython -m line_profiler
CLI apps;config
argument toline_profiler.explicit_profiler.GlobalProfiler.__init__()
,line_profiler.line_profiler.LineProfiler.print_stats()
, andline_profiler.line_profiler.show_text()
and.show_func()
; orLINE_PROFILER_RC
environment variable.Failing that, the config file is looked up on the file system starting from the current directory. The filenames
pyproject.toml
andline_profiler.toml
(the latter taking priority) are checked, and if any of them is valid TOML it is chosen to load configs from. Otherwise, we check in the parent directory, and so on, until we reach the file-system/drive root.The looked up file (if any) is then merged with the default configs (
line_profiler/rc/line_profiler.toml
) to form the final configuration table, which is guaranteed to contain the same subtables and keys as the default one.Motivation
In #323's discussion (comments 1, 2, 3), it was suggested that TOML config files can be used to:
kernprof -l -p <script> <script>
, andline_profiler.explicit_profiler.GlobalProfiler.show_config
configurable.It was also suggested that all config options used by this package be placed under the
tool.line_profiler
namespace.Code changes
line_profiler/rc
:New subpackage for containing config files (currently only
line_profiler.toml
)line_profiler/cli_tools.py
:New module for common functions shared between
kernprof
andpython -m line_profiler
line_profiler/toml_config.py
:New module for finding and loading configs from TOML files
get_config()
: load the configs from an explicitly-supplied TOML file, the file discovered from the environment, or the default fileget_default_config()
: load the configs from the default TOML file (line_profiler/line_profiler.toml
)kernprof.py
,line_profiler/line_profiler.py
:line_profiler.cli_tools
--config
(and-c
only forline_profiler/line_profiler.py
sincekernprof -c
is taken (ENH: auto-profilestdin
or literal snippets #338)) for specifying a TOML config file--no-config
for disabling loading of TOML configs (that isn't the default file)--some-flag
(action='store_true'
or'store_false'
):None
and the action replaced with'store_const'
or'store'
, so that:'store_const'
s, which allow them to be concatenated--some-flag=false --other-flag
, which resolves intoNamespace(some_flag=False, other_flag=True)
)--no-some-flag
is added--help
output-V
/--version
;-c
/--config
/--no-config
;-m
) now have corresponding TOML entries (in[tool.line_profiler.kernprof]
and[tool.line_profiler.cli]
) and load their defaults therefromkernprof.py
:__doc__
--prof-imports
to better reflect its functionconst
(value stored when the flag is passed without an argument) of the-i
/--output-interval
option, since the old value (0
) is un-intuitively equivalent to disabling itRepeatedTimer
line_profiler/line_profiler.py
:config
toLineProfiler.print_stats()
,show_func()
, andshow_text()
so that the output column widths can be customizedshow_func()
with the config table[tool.line_profiler.show.column_widths]
line_profiler/explicit_profiler.py
:config
toGlobalProfiler.__init__()
so that configs can be explicitly specifiedGlobalProfiler.setup_config
,.write_config
,.show_config
, and.output_prefix
with the config tables[tool.line_profiler.setup]
,[.write]
, and[.show]
Packaging changes
line_profiler/rc/line_profiler.toml
:New global-default config file
requirements/runtime.txt
:Updated so that
tomli
is installed for Python versions (< 3.11) withouttomllib
in thestdlib
setup.py
,MANIFEST.in
:Updated so that
line_profiler/rc/line_proiler.toml
is included in both wheels and source distributionsTest suite changes
tests/test_toml_config.py
:New tests for
line_profiler/toml_config.py
test_environment_isolation()
:Test that tests in this test module are reasonably isolated from its environment (env variables and file system)
test_default_config_deep_copy()
:Test that each call to
line_profiler.toml_config.get_default_config()
returns a fresh and separate set of configstest_table_normalization()
:Test that config files with missing and/or extra keys are normalized into the same layout as the default file
test_malformed_table()
:Test that malformed config files (e.g. one which sets
[tool.line_profiler.show.column_widths]
to an array, instead of the expected table result in aValueError
test_config_lookup_hierarchy()
:Test that the choice of config file follows the resolution scheme outlined in Specification and discovery
tests/test_explicit_profile.py
:tempfile.TemporaryDirectory
to ensure the removal of the temporary directoriestest_explicit_profiler_with_customized_config()
:New test for the configuration of
line_profiler.explicit_profiler.GlobalProfiler
(i.e.@line_profiler.profile
) via TOML config filestests/test_autoprofile.py
:tempfile.TemporaryDirectory
to ensure the removal of the temporary directoriestest_autoprofile_with_customized_config()
:New test for the configuration of
kernprof
,python -m line_profiler
, andline_profiler.line_profiler.show_text()
via TOML config filestest_autoprofile_with_no_config()
:New test for disabling user-config loading in
kernprof
andpython -m line_profiler
via the--no-config
flagtests/test_cli.py
:test_boolean_argument_help_text()
:New test for the augmented help texts generated by
line_profiler.cli_utils.add_argument()
for boolean optionstest_boolean_argument_parsing()
:New test for the parsing of boolean options generated by
line_profiler.cli_utils.add_argument()
Caveats
This PR further couples
kernprof
toline_profiler
. But it's probably fine, given that:kernprof
erroring out in non--l
, non--b
mode in Python 3.12+ #326, which madekernprof.contextualProfiler
directly dependent online_profiler.profiler_mixin.ByCountProfilerMixin
.kernprof
is dependent online_profiler
and making the former (somewhat) usable without the latter was to avoid necessitating the configuration of C(-ython) compilation. But since thw whole package can now just bepip install
-ed with no additional configuration, it's a mostly solved problem.Acknowledgement
The idea for the PR originated with @Erotemic.