-
Notifications
You must be signed in to change notification settings - Fork 9
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
PERF: refactor atef.bin cli entrypoint to defer imports #256
Conversation
…ty until subcommand is invoked
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.
This seems reasonable to me and is a big speedup due to some of the slower imports in some of the code paths. My only in-line review comment is a typo check.
I have mixed feelings about the various variants/options for a subparser scheme like this one. The previous arrangement had some performance problems but functionally made clear separations between the implementations of the different subcommands. Part of me wonders if there was a way to keep the subparsers a little more separate without needing to change so much, something like:
atef.bin.subcmd
import argparse
def build_parser():
...
def main(**kwargs):
from .subcmd_main import subcmd_main
subcmd_main(**kwargs)
And then the overall structure doesn't change too much and we still import a bunch of subparsers but the heavy imports are deferred and the subparsers aren't collocated.
But maybe there are even cleaner ways as well.
This is all just musing. I'm happy to merge with the typo fixed.
|
||
Maintenance | ||
----------- | ||
- improves the performance of the CLI entrypoint, defering functional imports as longas possible |
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.
- improves the performance of the CLI entrypoint, defering functional imports as longas possible | |
- improves the performance of the CLI entrypoint, deferring functional imports as long as possible |
Hmm I might like that formulation more. I'll give that a whirl |
superceded by #257 |
Description
atef.bin
to defer imports until absolutely necessaryIn more detail:
build_arg_parser
type functions to a module separate from the subcommand logicatef.bin.main
now grabs those parsers to create the top-level parser, avoiding the meaty functional importsmain
sub-commands are given an additional layer of wrapping, andatef.bin.main:main
now calls a partial function to get theatef.bin.<subcommand>:main
function, which then gets fed the parsed kwargs.Motivation and Context
I keep vendoring this cli hook and never improving it. This was more of a proof to myself that it could work. If we like it I can start using it elsewhere or even maybe cookiecutter it.
I should really work on more important tasks but I refactored it in my brain during a post-dinner walk and wanted to put it to paper
How Has This Been Tested?
Before
$ time atef config -h ... real 0m8.487s user 0m6.721s sys 0m1.158s
After
$ time atef config -h ... real 0m0.518s user 0m0.184s sys 0m0.075s
Aside from this, lots of interactive testing to make sure all the entrypoints are working.
Where Has This Been Documented?
This PR
Pre-merge checklist
docs/pre-release-notes.sh
and created a pre-release documentation page