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

Implement 2-phase parser with greedy subparsers #12910

Merged
merged 10 commits into from
Jul 24, 2023

Conversation

kenodegard
Copy link
Contributor

@kenodegard kenodegard commented Jul 20, 2023

Description

Splits CLI parser into a 2-phase parser. Phase 1 only parses conda's options (e.g., conda --json, conda --no-plugins, etc.) allowing us to disable plugins before generating the complete parser for phase 2. A benefit of the 2-phase parser is any option parsed in phase 1 can appear anywhere on the CLI, so the following are identical:

conda --json doctor
conda doctor --json

Note that conda doctor has not explicitly defined --json as one of its arguments:

def configure_parser(parser: ArgumentParser):
parser.add_argument(
"-v",
"--verbose",
action="store_true",
help="Generate a detailed environment health report.",
)
add_parser_help(parser)
add_parser_prefix(parser)

In addition to the 2-phase parser we also implement a custom argparse._SubParserAction that will greedily consume all unmatched arguments, this is the change that resolves the argparse.REMAINDER issue.

Resolves #12906

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@kenodegard kenodegard self-assigned this Jul 20, 2023
@kenodegard kenodegard requested a review from a team as a code owner July 20, 2023 18:18
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jul 20, 2023
@kenodegard kenodegard linked an issue Jul 20, 2023 that may be closed by this pull request
2 tasks
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Just some naming issues, interesting approach, I like it!

conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
conda/cli/main.py Outdated Show resolved Hide resolved
conda/cli/main.py Outdated Show resolved Hide resolved
conda/cli/main.py Outdated Show resolved Hide resolved
conda/cli/main.py Outdated Show resolved Hide resolved
conda/cli/conda_argparse.py Outdated Show resolved Hide resolved
@kenodegard
Copy link
Contributor Author

------------------------------------------------------------------------------------------ benchmark: 6 tests ------------------------------------------------------------------------------------
Name (time in ms)                   Version     Min             Max             Mean            StdDev          Median          IQR             Outliers    OPS                 Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_benchmark[conda --help]        (23.5.x)    3.8577 (1.04)   26.0305 (1.0)   4.6298 (1.0)    2.1254 (1.0)    4.3422 (1.03)   0.5957 (1.05)   2;5         215.9939 (1.0)      200     1
test_benchmark[conda --help]        (23.7.x)    5.2500 (1.41)   30.0415 (1.15)  6.0925 (1.32)   3.1933 (1.50)   5.4785 (1.29)   0.5684 (1.0)    4;5         164.1363 (0.76)     200     1

test_benchmark[conda info --help]   (23.5.x)    3.7122 (1.0)    33.0895 (1.27)  4.9982 (1.08)   3.4190 (1.61)   4.2315 (1.0)    0.7391 (1.30)   9;14        200.0713 (0.93)     204     1
test_benchmark[conda info --help]   (23.7.x)    4.8267 (1.30)   30.9138 (1.19)  5.6650 (1.22)   3.3532 (1.58)   5.0277 (1.19)   0.5813 (1.02)   4;4         176.5225 (0.82)     200     1

test_benchmark[conda doctor --help] (23.5.x)    4.9061 (1.32)   26.1723 (1.01)  6.0351 (1.30)   2.8967 (1.36)   5.3748 (1.27)   0.7109 (1.25)   6;10        165.6978 (0.77)     200     1
test_benchmark[conda doctor --help] (23.7.x)    4.7010 (1.27)   29.7277 (1.14)  5.4190 (1.17)   2.8634 (1.35)   4.9014 (1.16)   0.5848 (1.03)   3;3         184.5363 (0.85)     200     1
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

@travishathaway
Copy link
Contributor

travishathaway commented Jul 21, 2023

@kenodegard, those performance statistics don't look too good 😦... If I'm reading it correctly, we're seeing a 32% slow down for the mean run times of conda --help?

@travishathaway
Copy link
Contributor

travishathaway commented Jul 21, 2023

Also, another concern I have with the output of conda --help is that it makes it seem like conda build is a legacy command:

commands:
  The following built-in and plugins subcommands are available.

  COMMAND
    build             (legacy) Invoke `conda-build`.
    clean             Remove unused packages and caches.
    compare           Compare packages between conda environments.
    config            Modify configuration values in .condarc.
    content-trust     (legacy) Invoke `conda-content-trust`.
    convert           (legacy) Invoke `conda-convert`.
    create            Create a new conda environment from a list of specified packages.
    debug             (legacy) Invoke `conda-debug`.
    develop           (legacy) Invoke `conda-develop`.
    doctor            Display a health report for your environment.
    env               (legacy) Invoke `conda-env`.
    glist             Lists guarded environments
    guard             Guard environments so changes are not accidentally made to them
    index             (legacy) Invoke `conda-index`.
    info              Display information about current conda install.
    init              Initialize conda for shell interaction.
    inspect           (legacy) Invoke `conda-inspect`.
    install           Install a list of packages into a specified conda environment.
    list              List installed packages in a conda environment.
    metapackage       (legacy) Invoke `conda-metapackage`.
    notices           Retrieve latest channel notifications.
    package           Create low-level conda packages. (EXPERIMENTAL)
    remove (uninstall)
                      Remove a list of packages from a specified conda environment.
    rename            Rename an existing environment.
    render            (legacy) Invoke `conda-render`.
    run               Run an executable in a conda environment.
    search            Search for packages and display associated information using the MatchSpec format.
    server            (legacy) Invoke `conda-server`.
    skeleton          (legacy) Invoke `conda-skeleton`.
    update (upgrade)  Update conda packages to the latest compatible version.
    verify            (legacy) Invoke `conda-verify`.

Which isn't the case. I don't think the underlying implementation mechanism for how the subcommands are discovered really matters at all to our end users. Does it make sense to simply remove the (legacy) bit from the beginning of those descriptions?

Furthermore, those descriptions don't really make sense. Do we really want users to invoke conda-build and not conda build? Maybe it makes more sense to just say, run conda build --help for more information or just leave it blank.

@beeankha beeankha added source::anaconda created by members of Anaconda, Inc. plugins pertains to a plugin/subcommand labels Jul 21, 2023
@kenodegard
Copy link
Contributor Author

Some more benchmarks:

Data
----------------------------------------------------------------------------------------------- benchmark: 16 tests --------------------------------------------------------------
Name (time in ms)                                   Version    Min      Max         Mean            StdDev      Median      IQR         Outliers    OPS         Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_benchmark[conda --help]                        (23.5.x)   3.9752   28.9061     4.7374 (1.00)   2.6204      4.2982      0.5217      4;9         211.0875    300     1
test_benchmark[conda --help]                        (23.7.x)   5.1655   93.5507     7.0028 (1.48)   8.4186      5.6585      0.5356      7;14        142.8007    300     1
test_benchmark[conda --help]                        (greedy)   4.9201   33.1507     5.8390 (1.23)   3.3559      5.2385      0.6266      6;8         171.2624    300     1
test_benchmark[conda --help --no-plugins]           (greedy)   4.7789   31.7847     5.5726 (1.18)   3.2082      5.0274      0.5521      5;8         179.4500    300     1

test_benchmark[conda --version]                     (23.5.x)   3.5387   27.2832     4.2292 (1.00)   2.6441      3.7799      0.5345      4;6         236.4530    300     1
test_benchmark[conda --version]                     (23.7.x)   4.5992   181.1767    6.4743 (1.53)   14.4881     4.9511      0.3863      3;10        154.4574    300     1
test_benchmark[conda --version]                     (greedy)   4.2801   30.3349     5.1930 (1.23)   3.1626      4.6577      0.5406      5;8         192.5682    300     1
test_benchmark[conda --version --no-plugins]        (greedy)   4.2721   29.6854     5.0365 (1.19)   3.1039      4.4839      0.5583      5;6         198.5491    300     1

test_benchmark[conda doctor --help]                 (23.5.x)   4.9180   29.4194     5.7063 (1.00)   2.6698      5.2294      0.6433      4;7         175.2454    300     1
test_benchmark[conda doctor --help]                 (23.7.x)   4.7665   352.9111    6.3619 (1.11)   20.0773     5.1069      0.3781      1;9         157.1853    300     1
test_benchmark[conda doctor --help]                 (greedy)   4.4081   29.4150     5.1191 (0.90)   2.8167      4.6499      0.5951      4;4         195.3460    300     1
test_benchmark[conda doctor --help --no-plugins]    (greedy)   4.3973   30.5469     5.2155 (0.91)   3.1823      4.6731      0.4896      5;6         191.7362    300     1

test_benchmark[conda info --help]                   (23.5.x)   3.7842   27.2494     4.4591 (1.00)   2.6149      3.9770      0.5405      4;10        224.2617    300     1
test_benchmark[conda info --help]                   (23.7.x)   4.8929   213.9390    5.9831 (1.34)   12.0492     5.2001      0.3397      1;7         167.1371    300     1
test_benchmark[conda info --help]                   (greedy)   4.5417   32.6828     5.3409 (1.20)   3.1902      4.7561      0.6104      5;7         187.2336    300     1
test_benchmark[conda info --help --no-plugins]      (greedy)   4.5604   28.6841     5.3393 (1.20)   3.0553      4.7544      0.6119      5;10        187.2897    300     1
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

using 23.5.x as the baseline:

  • 23.7.x sees a 10-50% slowdown (~0.5-2ms) for builtin commands and plugin commands
  • this PR sees a 20% slowdown (~1ms) for builtin commands and a 10% speedup (~0.5ms) for plugin commands

I interpret this as meaning the slowdown was introduced in some other change since 23.5.x

@kenodegard kenodegard merged commit 0a2689b into conda:23.7.x Jul 24, 2023
67 checks passed
@kenodegard kenodegard deleted the greedy-subparser branch July 24, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA plugins pertains to a plugin/subcommand source::anaconda created by members of Anaconda, Inc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Subcommand arguments are incorrectly parsed
6 participants