New wrapped CLI using click - #4961
Conversation
|
This looks kind of nice, but I'm not a big fan of introducing a required 3rd party dependency. Is there any way we can support this opt-in, for example by defining an environment variable like |
|
We could make use of probably not only for this but also for all the optional dependencies we specify in Then we add a check on top of |
click CLI wrapper around normal CLIeb2 CLI using click
|
So for now the The approach of using an ENV variable i think make sense if we decide to merge |
- Fixed beahvore of defaults with `store_or_None` - Fixed checking defaults vs list/tuple instead of flattened value - Added checks for list values in covert comming from the defaults
a07c32e to
681e781
Compare
681e781 to
755d496
Compare
Co-authored-by: Kenneth Hoste <kenneth.hoste@ugent.be>
…ramework into feature-click_cli
b8c5791 to
0267a3c
Compare
…` is used. Allow list values to be False/None for `store_or_XXX`
Also modified the end2end tests to do
|
| except ImportError: | ||
| def eb(*args, **kwargs): | ||
| """Placeholder function to inform the user that `click` is required.""" | ||
| main_with_hooks() |
There was a problem hiding this comment.
I doesn't make much sense to me to specify that a Click-based CLI should be used, to then fall back to a non-Click CLI when click is not found...
If click is not found, then we should fail with an error (and print a hint about unsetting $EB_CLI_CLICK to opt-out of using a Click-based CLI
| # | ||
| # You should have received a copy of the GNU General Public License | ||
| # along with EasyBuild. If not, see <http://www.gnu.org/licenses/>. | ||
| # # |
There was a problem hiding this comment.
@Crivella please add a docstring with some info on what this provides, add yourself as author
| ]) | ||
|
|
||
| from .options import EasyBuildCliOption, EasyconfigParam | ||
| from easybuild.tools.version import this_is_easybuild |
There was a problem hiding this comment.
Why not do these imports on top?
There was a problem hiding this comment.
The second one can be outside, the first i would leave inside so that the only check we need w.r.t. click is in this file.
Originally it would cause the fallback to the normal CLI to not work (since something was still trying to import click) now it would cause an ModuleNotFound error before our new raise that replaced the CLI fallback
| decl = f"--{self.name}/--disable-{self.name}" | ||
| elif isinstance(self.default, (list, tuple)): | ||
| kwargs['multiple'] = True | ||
| kwargs['type'] = click.STRING |
There was a problem hiding this comment.
add else to avoid that we don't handle unknown option types?
There was a problem hiding this comment.
Had to check since it has been a while, added a print to check what this would hit:
Warning: Cannot infer type for option bwrap-installpath with default value /home/crivella/.local/easybuild/bwrap, treating as string
Warning: Cannot infer type for option module-naming-scheme with default value EasyBuildMNS, treating as string
Warning: Cannot infer type for option packagepath with default value /home/crivella/.local/easybuild/packages, treating as string
Warning: Cannot infer type for option subdir-data with default value data, treating as string
Warning: Cannot infer type for option subdir-modules with default value modules, treating as string
Warning: Cannot infer type for option subdir-software with default value software, treating as string
Warning: Cannot infer type for option subdir-user-modules with default value None, treating as string
Warning: Cannot infer type for option suffix-modules-path with default value all, treating as string
Warning: Cannot infer type for option container-image-name with default value None, treating as string
Warning: Cannot infer type for option container-tmpdir with default value None, treating as string
Warning: Cannot infer type for option create-index with default value None, treating as string
Warning: Cannot infer type for option dump-test-report with default value test_report.md, treating as string
Warning: Cannot infer type for option review-pr-filter with default value None, treating as string
Warning: Cannot infer type for option test-report-env-filter with default value None, treating as string
Warning: Cannot infer type for option dep-graph with default value None, treating as string
Warning: Cannot infer type for option search with default value None, treating as string
Warning: Cannot infer type for option search-filename with default value None, treating as string
Warning: Cannot infer type for option search-short with default value None, treating as string
Warning: Cannot infer type for option easystack with default value None, treating as string
Warning: Cannot infer type for option job-backend-config with default value None, treating as string
Warning: Cannot infer type for option job-output-dir with default value /home/crivella, treating as string
Warning: Cannot infer type for option job-target-resource with default value None, treating as string
Warning: Cannot infer type for option backup-modules with default value None, treating as string
Warning: Cannot infer type for option check-ebroot-env-vars with default value warn, treating as string
Warning: Cannot infer type for option deprecated with default value None, treating as string
Warning: Cannot infer type for option detect-loaded-modules with default value warn, treating as string
Warning: Cannot infer type for option easyblock with default value None, treating as string
Warning: Cannot infer type for option env-for-shebang with default value /usr/bin/env, treating as string
Warning: Cannot infer type for option group with default value None, treating as string
Warning: Cannot infer type for option http-header-fields-urlpat with default value None, treating as string
Warning: Cannot infer type for option minimal-build-env with default value CC:gcc,CXX:g++, treating as string
Warning: Cannot infer type for option module-cache-suffix with default value None, treating as string
Warning: Cannot infer type for option mpi-cmd-template with default value None, treating as string
Warning: Cannot infer type for option optarch with default value None, treating as string
Warning: Cannot infer type for option rpath-override-dirs with default value None, treating as string
Warning: Cannot infer type for option software-commit with default value None, treating as string
Warning: Cannot infer type for option sysroot with default value None, treating as string
Warning: Cannot infer type for option umask with default value None, treating as string
Warning: Cannot infer type for option zip-logs with default value gzip, treating as string
Warning: Cannot infer type for option package-tool with default value fpm, treating as string
Warning: Cannot infer type for option package-tool-options with default value , treating as string
Warning: Cannot infer type for option package-type with default value rpm, treating as string
Warning: Cannot infer type for option package-release with default value 1, treating as string
Warning: Cannot infer type for option aggregate-regtest with default value None, treating as string
Warning: Cannot infer type for option regtest-output-dir with default value None, treating as string
Warning: Cannot infer type for option amend with default value None, treating as string
Warning: Cannot infer type for option software-name with default value None, treating as string
Warning: Cannot infer type for option software-version with default value None, treating as string
Warning: Cannot infer type for option toolchain-name with default value None, treating as string
Warning: Cannot infer type for option toolchain-version with default value None, treating as string
Warning: Cannot infer type for option try-amend with default value None, treating as string
Warning: Cannot infer type for option try-software-name with default value None, treating as string
Warning: Cannot infer type for option try-software-version with default value None, treating as string
Warning: Cannot infer type for option try-toolchain-name with default value None, treating as string
Warning: Cannot infer type for option try-toolchain-version with default value None, treating as string
Warning: Cannot infer type for option unittest-file with default value None, treating as string
this are all variables with no specific type.
When we do not hit any of those if/else type will be left as None and click will treat the argument as a string eg:

This only come into play for the auto-completion (nothing will be suggested), not sure if it is better to enforce click.STRING as a type, but it is standard to not specify a type with click
| meta: Dict = None | ||
| lst: List = None | ||
|
|
||
| def __post_init__(self): |
| return res | ||
|
|
||
|
|
||
| class DelimitedString(click.ParamType): |
There was a problem hiding this comment.
missing docstrings in methods below
| extracter = OptionExtracter(go_args=[]) | ||
|
|
||
|
|
||
| class DelimitedPathList(click.Path): |
There was a problem hiding this comment.
missing docstrings in methods below
| 'buildpath', | ||
| 'containerpath', | ||
| 'installpath', | ||
| 'sourcepath', |
There was a problem hiding this comment.
bwrap_installpath missing?
packagepath, repositorypath too?
Why not keep these alphabetically sorted?
Also, how will we keep this in sync as new options get added?
There was a problem hiding this comment.
This is for convenience to allow the autocompletion to treat something that should be a string for optparse as a path to a file/directory.
Also, how will we keep this in sync as new options get added?
Right now there is no easy way to maintain this but adding stuff manually.
With a proper click CLI we will use the correct type from the outset
Why not keep these alphabetically sorted?
I think i tried to divide them logically while i was manually adding them but fine also having them alphabetically sorted
There was a problem hiding this comment.
Also repositorypath is flagged as strlist and passes a list as a default so it is not compatible with click.Path the way we are setting it now
easybuild-framework/easybuild/tools/options.py
Lines 719 to 722 in daf0bac
…not parsable as a `click.Path`
…ed in the conversion to click
Enabling
Using pip and optional python dependencies:
pip install easybuild-framework[eb_click]How it works
EasyBuildOptionsand use them to generate a CLI withclickclick+rich_clickto have a fancier looking CLIclickis only used for the nicer help and auto completion, but the parameters are still passed as is to the normal parserAutocomplete
From click docs add the following to the
.bashrcor venv activation script.Alternatively see aiida-core EC to activate it as a module
Advantages
bashzshfishshellsTODO
Maybe
print_msg#5125)clicknatively and divide the commands in subgroups to have an easier to readhelp(Discussion moved to Modern CLI for easybuild #5122)How would this look like
eb --help
Error
Normal build run
Opt in/out
the new behavior will only be turned on if

clickis installed, otherwise the old one will be used.