Skip to content
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

Initial support for argcomplete for KVArgParseConfigLoader #811

Merged
merged 30 commits into from Dec 19, 2022

Conversation

azjps
Copy link
Collaborator

@azjps azjps commented Dec 7, 2022

After enabling argcomplete shell completion for a traitlets.Application based script (e.g. via activate-global-python-argcomplete), this commit sets up tab auto-completion for command-line flags and aliases. (Note that argcomplete would be completely optional and not a dependency.)

Argcomplete completers can be added for a trait by using an "argcompleter" metadata tag, which should be a function that takes keyword arguments (passed down from argcomplete) and returns a list of string completions. Completers are set up for Bool ("true", "false", "1", "0") & Enum.

This commit does not yet add general support for arbitrary class traits of the form --Class.trait=xyz, as these are currently not added to the ArgumentParser instance, but rather directly parsed from sys.argv. It is probably possible to add this support for the classes in Application.classes. IMHO, the most user-friendly way of doing this would be to allow --<TAB> to also show the completions --Class1. --Class2. and so forth, enumerating the Configurable classes but not spamming all of the available traits. Only once the class has been completed out e.g. --Class1.<TAB> would the traits of the class become completable. While this should be possible with argcomplete I haven't looked closely enough to figure out what the easiest way to accomplish this would be.

Issue: #539

Example:

~/dev/traitlets$ python examples/myapp.py --[TAB]
--debug      --disable    --enable     --enabled    --help       --i          --j          --log_level  --mode       --name       --running

~/dev/traitlets$ python examples/myapp.py --running [TAB]
0      1      false  true

~/dev/traitlets$ python examples/myapp.py --running true --[TAB]
--debug      --disable    --enable     --enabled    --help       --i          --j          --log_level  --mode       --name       --running

~/dev/traitlets$ python examples/myapp.py --running true --mode o[TAB]
off    on     other

If this idea seems good to pursue, I can add documentation, update dev dependencies, and so forth.

@blink1073
Copy link
Member

Very cool! I think it makes sense to add argcomplete as a test dependency. I am also 👍🏼 on completing the class names, that would be very helpful IMO.

azjps and others added 4 commits December 9, 2022 20:24
After enabling argcomplete shell completion for a traitlets.Application
based script (e.g. via activate-global-python-argcomplete), this commit
sets up tab auto-completion for command-line flags and aliases.

Argcomplete completers can be added for a trait by using an "argcompleter"
metadata tag, which should have a function that takes keyword arguments
(passed down from argcomplete) and returns a list of string completions.
Completers are also set up for Bool ("true", "false", "1", "0") & Enum.

This commit does *not* add general support for arbitrary class traits
of the form --Class.trait=xyz, as these are currently not added to
the ArgumentParser instance, but rather directly parsed. It is probably
possible to add this support for the classes in Application.classes.

Issue: ipython#539

Example:

~/dev/traitlets$ python examples/myapp.py --[TAB]
--debug      --disable    --enable     --enabled    --help       --i          --j          --log_level  --mode       --name       --running

~/dev/traitlets$ python examples/myapp.py --running [TAB]
0      1      false  true

~/dev/traitlets$ python examples/myapp.py --running true --[TAB]
--debug      --disable    --enable     --enabled    --help       --i          --j          --log_level  --mode       --name       --running

~/dev/traitlets$ python examples/myapp.py --running true --mode o[TAB]
off    on     other
This custom finder mainly adds 2 functionalities:

1. When completing options, it will add --Class. to the
list of completions, for each class in Application.classes
that could complete the current option.

2. If it detects that we are currently trying to complete
an option related to --Class., it will add the corresponding
config traits of Class to the ArgumentParser instance,
so that the traits' completers can be used. (This is currently
done in a bit of a hacky manner.)

Note that we are avoiding adding all config traits of all classes
to the ArgumentParser, which would be easier but would add more
runtime overhead and would also make completions
appear more spammy. It also does not support nested class
options like --Class1.Class2.trait.

Example:

~/dev/traitlets$ examples/myapp.py --mode on --[TAB]
--Application.  --Foo.          --debug         --enable        --help          --j             --mode          --running
--Bar.          --MyApp.        --disable       --enabled       --i             --log_level     --name
~/dev/traitlets$ examples/myapp.py --mode on --F[TAB]
~/dev/traitlets$ examples/myapp.py --mode on --Foo.[TAB]
--Foo.i     --Foo.j     --Foo.mode  --Foo.name
~/dev/traitlets$ examples/myapp.py --mode on --Foo.m[TAB]
~/dev/traitlets$ examples/myapp.py --mode on --Foo.mode [TAB]
~/dev/traitlets$ examples/myapp.py --mode on --Foo.mode o[TAB]
off    on     other
Added an example application for testing argcomplete,
under examples/argcomplete_app.py, with several examples
of completions provided in the docstring.

Fixed using completers

--Class.trait arg1 arg2 [TAB]

for config traits with nargs/multiplicity="+". Note that
currently traitlets does not support multiplicity even though
it is used in the code; refer to issue GH#690 for discussion.

Add more comments since we're using argcomplete internals,
and add argcomplete as dependency for coverage/mypy tests.
Some other minor fixes such as minor mypy annotations fixes.

Another example: add # PYTHON_ARGCOMPLETE_OK to bin/ipython,
and tab-complete away:

    $ ipython --[TAB]
    --Application.               --help
    --BaseFormatter.             --i
    --BaseIPythonApplication.    --ignore-cwd
    --Completer.                 --init
    --HistoryAccessor.           --ipython-dir
    --HistoryManager.            --log-level
    --IPCompleter.               --logappend
    --InteractiveShell.          --logfile
    --InteractiveShellApp.       --m
    ...

    $ ipython --gui=[TAB]
    asyncio  gtk      gtk3     pyglet   qt4      tk
    glut     gtk2     osx      qt       qt5      wx

To-do still: support subcommands. This may still take some work
as traitlets does subcommand parsing independently of argparse.
@azjps azjps force-pushed the argcomplete branch 2 times, most recently from 523ec41 to c0c6aab Compare December 9, 2022 10:39
@azjps
Copy link
Collaborator Author

azjps commented Dec 9, 2022

Turns out argcomplete doesn't currently have built-in support to partially complete options, in particular if one adds --Class. as an argparse option, then argcomplete will add a space afterwards when its tab-completed, which is user-unfriendly.

I managed to hack around a bit and directly add all of the config traits as parser.add_argument("--Class.<trait>"), but only when we detect that something that looks like --Class.* or its following arguments are being completed. It does use argcomplete's internals so maybe there's something better that could be done.

I've added an example script examples/argcomplete_app.py, the docstring has different completions I've tried to test.

Another example: add # PYTHON_ARGCOMPLETE_OK as the second line for bin/ipython, and tab-complete away:

$ ipython --
--Application.               --StoreMagics.               --config                     --m                          --nosep
--BaseFormatter.             --TerminalIPythonApp.        --confirm-exit               --matplotlib                 --pdb
--BaseIPythonApplication.    --TerminalInteractiveShell.  --debug                      --no-autoedit-syntax         --pprint
--Completer.                 --autocall                   --ext                        --no-autoindent              --profile
--HistoryAccessor.           --autoedit-syntax            --gui                        --no-automagic               --profile-dir
--HistoryManager.            --autoindent                 --help                       --no-banner                  --pylab
--IPCompleter.               --automagic                  --i                          --no-color-info              --quick
--InteractiveShell.          --banner                     --ignore-cwd                 --no-confirm-exit            --quiet
--InteractiveShellApp.       --c                          --init                       --no-ignore-cwd              --simple-prompt
--LoggingMagics.             --cache-size                 --ipython-dir                --no-pdb                     --term-title
--PlainTextFormatter.        --classic                    --log-level                  --no-pprint
--ProfileDir.                --color-info                 --logappend                  --no-simple-prompt
--ScriptMagics.              --colors                     --logfile                    --no-term-title

$ ipython --gui=
asyncio  glut     gtk      gtk2     gtk3     osx      pyglet   qt       qt4      qt5      tk       wx

$ ipython --gui g
glut  gtk   gtk2  gtk3

$ ipython --InteractiveShellApp.
--InteractiveShellApp.code_to_run                         --InteractiveShellApp.hide_initial_ns
--InteractiveShellApp.exec_PYTHONSTARTUP                  --InteractiveShellApp.ignore_cwd
--InteractiveShellApp.exec_files                          --InteractiveShellApp.matplotlib
--InteractiveShellApp.exec_lines                          --InteractiveShellApp.module_to_run
--InteractiveShellApp.extensions                          --InteractiveShellApp.pylab
--InteractiveShellApp.extra_extension                     --InteractiveShellApp.pylab_import_all
--InteractiveShellApp.file_to_run                         --InteractiveShellApp.reraise_ipython_extension_failures
--InteractiveShellApp.gui

Sub-commands might be tricky since IIRC they're handled by traitlets and not by argparse (and thus argcomplete won't know about them), I haven't started looking into it yet.

@blink1073
Copy link
Member

Looking good so far!

@azjps
Copy link
Collaborator Author

azjps commented Dec 11, 2022

Unfortunately subcommand completion might not be all that viable, so I'll leave it for a separate PR that might not happen. I'll document the issues I encountered so there's a record:

  1. argcomplete's strategy is to call the python script with no arguments e.g. len(sys.argv) == 1, run until the ArgumentParser is constructed and determine what completions are available. On the other hand, traitlet's subcommand-handling strategy is to check sys.argv[1] and see if it matches a subcommand, and if so then dynamically load the subcommand app and initialize it with sys.argv[1:]. So there's this dichotomy where argcomplete tracks things by environment variables (it gets passed the command-line tokens through environment variables $COMP_LINE, $COMP_POINT) , and traitlets tracks things by argv. This means that when argcomplete is doing its pass, there is no sys.argv[1] and the traitlets sub-app never gets created.
    a. We can hack around this and set argv = parse($COMP_LINE, $COMP_POINT) if $_ARGCOMPLETE to get traitlets to go down the correct code path of loading the subcommand app and constructing the appropriate ArgumentParser. But now, argcomplete will be parsing the wrong tokens since it will be looking at the original $COMP_LINE which from the perspective of the subapp has an extra positional argument, so we'd need to find some way to get argcomplete to ignore that. Which can be done by environment variable manipulation, but I've probably conveyed how hacky this is.
    b. Or maybe we can be lazy and just let argcomplete receive some extra positional arguments (the subcommands) at the start of the command. It does seem to work fine for vanilla cases.
  2. For completing the subcommands themselves, e.g. jupyter nb[TAB], we would need to tell argcomplete about the subcommands and to figure out when its completing a subcommand. Probably possible but again would require some argcomplete-specific handling and some care with nested subcommands.
  3. With jupyter specifically, it does a few things with argument parsing -- main() has some dedicated sys.argv/argparse handling, and Application.initialize() is overridden to dynamically os.execv("jupyter-{subc}") subcommands based on sys.argv[1]. So both of these and probably some other places would need equivalent argcomplete handling as mentioned above in 1a. And for 2 we would have to reverse check $PATH for anything starting with jupyter- to figure out candidate subcommands. But, it does kind of work when you carry through with it:
~$ jupyter lab -[TAB]
--Application.               --core-mode
--ConnectionFileMixin.       --debug
--ContentsManager.           --dev-mode
--FileContentsManager.       --expose-app-in-browser
--FileManagerMixin.          --gateway-url
--GatewayClient.             --generate-config
--GatewayKernelManager.      --help
--GatewayKernelSpecManager.  --ip
--JupyterApp.                --keyfile
--KernelManager.             --log-level
--KernelSpecManager.         --no-browser
--LabApp.                    --no-mathjax
# ...

In summary, subcommand handling is not very viable without some planning, and for jupyter would require coordinated changes in at least 2/3 of traitlets, jupyter, argcomplete. In the meantime I'll try to clean up this PR with test fixes & docs.

azjps and others added 2 commits December 12, 2022 11:18
argcomplete's strategy is to call the python script with
no arguments e.g. len(sys.argv) == 1, run until the ArgumentParser
is constructed and determine what completions are available.
On the other hand, traitlet's subcommand-handling strategy
is to check sys.argv[1] and see if it matches a subcommand,
and if so then dynamically load the subcommand app and initialize
it with sys.argv[1:].

Write a couple of helper functions to reconcile this by:

1. retrieving tokens from $COMP_LINES, etc, and setting it to argv
2. if traitlets descends into a subcommand, increment index passed
   via env var to argcomplete to mark where command starts

There's quite a few caveats to this approach. For example, it only
is evaluated currently when `App.initialize()` is passed with
`argv=None` (the default). If `argv` is explicitly passed, then
the `argcomplete`-specific handling is skipped currently.

More details in:
ipython#811 (comment)

Some additional minor cleanup with respect to subcommands typing,
examples, and documentation.
@blink1073
Copy link
Member

Sounds good, thanks again @azjps!

Add docs for argcomplete, and various fixes and improvements
to other docs such as examples for flags/aliases/subcommands.

Update docs for building docs.

Also fix a bug when argcomplete is not installed
* Fix ruff removing argcomplete import check
* Add example scripts corresponding to examples in docs
* Add new sections to docs to further explain Application
configuration, methods, and philosophy.

Closes: ipython#707, ipython#708, ipython#709, ipython#712
pre-commit-ci bot and others added 8 commits December 14, 2022 11:06
Borrow argcomplete's unit test fixtures to set up
unit testing for completion of Application commands.
Test a few cases of custom completers; caught a bug
with Enum.argcompleter()
Fix some errors from hatch run typing:test
and some other minor formatting / CI fixes.
argcomplete >= 2.0 was very recently released, and
changes internal output_stream to text mode instead
of bytes; fix unit test for this and some other minor fixes.
These tests run for me locally, but currently CI raises
OSError: Bad file descriptor. Something with temp files perhaps?
@azjps
Copy link
Collaborator Author

azjps commented Dec 15, 2022

@blink1073 I think I've gotten into a style battle with pre-commit which re-adds some whitespace lines that causes ruff lint to fail with I001 Import block is un-sorted or un-formatted. 🤣

downstream_check > nbconvert is also failing, I'm not sure what this indicates?

While stashing my changes related to subcommand handling, I decided instead to just go ahead with it, its probably very fragile but it works for the simple examples at least.

I added some stuff to the config.rst docs and examples/ directory, haven't proofread it that closely though.

I've gotten some unit tests working for argcomplete & traitlets.Application which "mock" the environment variables being used while completing and redirecting the completion output to a TemporaryFile, its working in my local environments but failing in CI with OSError and a few other errors. I have to admit I don't really know what's going on there, but for now I've just marked them with pytest.mark.skip.

Besides these points, I'm satisfied with the state of these commits, probably the next thing I'll look into is opening PRs against the major traitlets-based projects (ipython, jupyter, etc) to actually enable argcomplete.

@blink1073
Copy link
Member

#814 fixes the linting issue

@blink1073
Copy link
Member

The nbconvert failure is unrelated. I'll take a look at the CI skipping, I'd rather not merge without that in place. It might be a bit before I get to it, I'm recovering from an illness.

@azjps
Copy link
Collaborator Author

azjps commented Dec 15, 2022

No worries. I re-enabled the tests and they seem to deterministically fail here, I've been trying to bisect towards the issue in #815 but haven't gotten a simpler example to fail yet 😞

TemporaryFile was causing some OSError(9) Bad file descriptor
on flush, not sure why but using StringIO seems to work
perfectly fine instead.
Only use StringIO instead of BytesIO for argcomplete >= 2.0.
Stop relying on SystemExit which might be messing with pytest,
instead raise a CustomError. Other minor doc fixes.
still trying to get pytest to not crash at the end ..
@azjps
Copy link
Collaborator Author

azjps commented Dec 16, 2022

Changing TemporaryFile -> StringIO was enough to get all the unit tests to pass, unfortunately pytest still crashes even though all tests pass. Possibly in coverage checking or something else and the errors are a bit difficult to scrutinize -- probably some side effect of argcomplete but having trouble narrowing it down

Edit: actually was able to narrow it down as per #815 -- argcomplete always calls debug_stream = os.fdopen(9, "w+"), I'm not exactly clear but it seems to clash with a file descriptor used by pytest especially when other plugins like coverage-py are involved. The only two reasonable solutions I can think of are:

  1. Get this fixed upstream in argcomplete (e.g. moving the debug stream manip into its own function, possibly one we can override)
  2. Use pytest-mock to patch os.fdopen

argcomplete==2.0.0 always calls fdopen(9, "w") to open a debug stream,
however this could conflict with file descriptors used by pytest
and lead to obscure errors. Since we are not looking at debug stream
in these tests, just mock this fdopen call out.
@blink1073
Copy link
Member

Use pytest-mock to patch os.fdopen

That seems reasonable. Great work!

pyproject.toml Outdated Show resolved Hide resolved
except ImportError:
# This module and its utility methods are written to not crash even
# if argcomplete is not installed.
class StubModule:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to be a bit defensive here and allow this module to be importable even if argcomplete is not installed, maybe there's a more common pattern that can be used here?

Copy link
Member

Choose a reason for hiding this comment

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

This is a nice approach, I'd say leave it.

Polish up a bit more docs about Application and commit
corresponding examples.
config.py files have get_config, load_subconfig injected
so have to get them ignored from pytest collection and
mypy checking.

Also minor, added some pathlib support to load_config_file().
Also link to some examples and add initial CHANGELOG entry.
CHANGELOG.md Outdated Show resolved Hide resolved
@blink1073
Copy link
Member

I'll manually update the release notes during the release to add your language for the documentation, inlining here:

### Enhancements

- Shell command-line tab-completion via `argcomplete` [#811](https://github.com/ipython/traitlets/pull/811) ([@azjps](https://github.com/azjps))

### Documentation

- Additional `Application` examples and docs [#811](https://github.com/ipython/traitlets/pull/811) ([@azjps](https://github.com/azjps))
``

@blink1073
Copy link
Member

If you beat me to it, please remove the changelog and update the branch, otherwise I'll do it tomorrow and cut a minor release. Thanks again!

azjps added a commit to azjps/ipython that referenced this pull request Dec 19, 2022
Set up shell command-line tab-completion using
argcomplete and ipython/traitlets#811

argcomplete supports following setuptools console_scripts
to the corresponding package's __main__.py to look for a
PYTHON_ARGCOMPLETE_OK marker.
@blink1073
Copy link
Member

https://github.com/ipython/traitlets/releases/tag/v5.8.0

azjps added a commit to azjps/jupyter_core that referenced this pull request Jan 28, 2023
Set up shell command-line tab-completion using argcomplete and
ipython/traitlets#811.

For subcommand handling, this will use list_subcommands() to
try to complete the first argument if it doesn't start with a "-",
and when a subcommand is detected, argcomplete will increment the
start of the command-line handling past the subcommand and drop into
the relevant traitlets.Application's argcomplete handling. All of
the major subcommands like "jupyter lab" directly use
traitlets.Application.launch_instance so this works pretty seamlessly.

However, completing commands that start with the hyphenated commands,
e.g. "jupyter-lab", would still require the addition of the
PYTHON_ARGCOMPLETE_OK marker in the respective projects' main scripts.

Note that argcomplete supports following setuptools console_scripts
to the corresponding main script to look for the PYTHON_ARGCOMPLETE_OK
marker.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants