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

Add hera CLI and implement generate yaml. #886

Merged
merged 12 commits into from
Dec 21, 2023
Merged

Conversation

DanCardin
Copy link
Contributor

@DanCardin DanCardin commented Dec 6, 2023

Pull Request Checklist

Description of PR
Per design outlined in #670 and #711

My personal usecase ideally requires generating for a whole folder at a time, but I only need output to stdout (Argo sidecar cmp plugin). Anything not in that critical path, I'm happy to remove if it's more challenging to agree on everything all at once.

  • Adds the hera cli console script
    • optional dependency, informs the user of the required extra if they use it
  • implements generate yaml

Notes:

  • Notable difference from the design outlined in the above issues: rather than --from as a required "option", I opted for it to be a positional argument for now. I can certainly change this back to --from if you feel strongly against it
  • I will certainly add tests once there's an indication that this looks roughly like what you'd want/accept
  • All of the help text/error messaging is just a first draft, feel free to suggest different prose.
  • I've implemented both file.py -> stdout, folder/ to stdout, file.py -> foo.yaml, and folder/ -> to_folder/
    • I think folder to folder is perhaps the most opinionated, in that it makes the decision that all source files get converted with the same name but a yaml suffix. It seems logical to me, but I could just as easily disallow this option.
  • I've chosen to use Typer here, because it was what Add CLI framework files #674 originally chose to use. I have a personal (very biased) preference towards Cappa, and I think it matches the style of Hera itself really well. With that said, Typer is the obviously more established option, so I'm not at all expecting to get buy-in to use Cappa here (just putting it out there 🤣).

src/hera/_cli/main.py Outdated Show resolved Hide resolved
@flaviuvadan flaviuvadan added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Dec 7, 2023
@flaviuvadan
Copy link
Collaborator

Thanks for the contribution @DanCardin! Also, first time contributor 🥳 Will review the PR this week

@sambhav
Copy link
Collaborator

sambhav commented Dec 8, 2023

@DanCardin thank you for this PR. Looks fantastic. Looking at Cappa, it certainly seems to fit very well with hera's use of pydantic. I am curious as to what a cappa based implementation would look like. I think my main inhibition is the long term support/maintenance of cappa and its maturity. Can you provide more details on what your plans around cappa are and if you have any examples that showcase it being used in a large-ish CLI?

@DanCardin
Copy link
Contributor Author

DanCardin commented Dec 8, 2023

Can you provide more details on what your plans around cappa are and if you have any examples that showcase it being used in a large-ish CLI?

Ideally cappa should be able to reproduce anything click(/typer)/argparse can do, but I dont have any specific API plans currently, beyond feature parity and anything users might request. I personally feel like the authoring experience is already greatly better than click/argparse/typer, but beauty is certainly in the eye of the beholder.

All of the examples I have of its use are either its own tests or private repos unfortunately. With that said, I feel like the design of cappa is such that the scale of the CLI is mostly irrelevant, since you're left with bare functions and classes you can interact with directly.

For reference, I quickly converted my PR from typer to cappa here: https://github.com/DanCardin/hera/tree/dc/cli-cappa, for a point of reference given an identical CLI interface. The code implementation is 95% identical, it's really just the CLI definition code that's different (although I think implementing the tests in cappa will be much simpler) as far as risk of choosing something and then switching goes. However certainly switching after the fact will yield completely different looking --help text, by default.

I think my main inhibition is the long term support/maintenance of cappa and its maturity.

That's totally fair. I never expected y'all to be interested in adopting a relatively unknown CLI library over one of the more popular/established alternatives. Totally up to y'all


Ignoring that for a minute, I could start implementing tests (perhaps as an additional comparison point between cappa/typer) as soon as the actual behavior (in particular wrt how files/folders interact) I happened implement is agreed to be what you're looking for, or not.

Copy link
Collaborator

@flaviuvadan flaviuvadan left a comment

Choose a reason for hiding this comment

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

This looks great @DanCardin!

I don't have any major meaningful feedback + saw Sam's feedback on Cappa so I will let Sam + Elliot provide the major input here

@DanCardin DanCardin marked this pull request as ready for review December 15, 2023 16:52
@DanCardin DanCardin force-pushed the dc/cli branch 3 times, most recently from d18fdaf to c5c7fe7 Compare December 15, 2023 17:01
@DanCardin
Copy link
Contributor Author

I'm not sure if what I've done for the tests is aligned with how you'd want to do CI. Namely that I excluded CLI tests from your main secton, and added a separate job for CLI-only tests, given that it's an optional extra, and the dependency wouldn't exist given your default poetry install.

For now, I've swapped the impl to the cappa version, given that I know much better how to implement fast, dependency-less tests with cappa, which enabled me to more quickly write the tests. I've stashed the original typer implementation sans tests here.

I can certainly swap back and convert the tests to whatever typer's interface would be for them if you decide that way.


Also not sure what you want to do regarding docs. I figured, given the relatively experimental nature of it thus far, you might want to leave it undocumented until it's more concrete. but if not, i can certainly add that myself as well.

@DanCardin
Copy link
Contributor Author

DanCardin commented Dec 15, 2023

Added note about the CLI being experimental. Also here's some screenshots of what it looks like currently:

Screenshot 2023-12-15 at 1 09 07 PM Screenshot 2023-12-15 at 1 09 16 PM Screenshot 2023-12-15 at 1 09 25 PM

EDIT:

sidebar, these failing CI builds seem very unrelated to my changes, i dont know what's going on there.

src/hera/__main__.py Outdated Show resolved Hide resolved
src/hera/_cli/base.py Outdated Show resolved Hide resolved
src/hera/_cli/base.py Outdated Show resolved Hide resolved
@elliotgunton
Copy link
Collaborator

elliotgunton commented Dec 18, 2023

@DanCardin awesome stuff, thank you! We're happy to trial out cappa, given it's a small CLI for now and looks like we can change over to typer without too much trouble if we hit too many walls. The testable functions is a big bonus!

The PR in general looks good (just went through with some nitpicks!), and happy with the choices - having the separate CLI installable and experimental warning when running looks like enough to cover our bases in terms of angry customers! 😆

Strange CI failures, taking a look!

src/hera/_cli/base.py Outdated Show resolved Hide resolved
poetry.lock Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e434f52) 79.8% compared to head (3ff59d2) 80.3%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #886     +/-   ##
=======================================
+ Coverage   79.8%   80.3%   +0.4%     
=======================================
  Files         45      49      +4     
  Lines       3736    3821     +85     
  Branches     759     777     +18     
=======================================
+ Hits        2984    3069     +85     
  Misses       557     557             
  Partials     195     195             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DanCardin
Copy link
Contributor Author

DanCardin commented Dec 19, 2023

mmm. Given how you're linting now, i guess it might perhaps be easier to add cappa as an unconditional dev dependency, run all tests normally without this separate only-cli-tests job, and then add a test that patches away the dependency and calls __main__.main().

Otherwise linting complains about cappa not existing on normal jobs, and the coverage checker is likely to complain about unconvered lines

EDIT: done this ^.

@DanCardin DanCardin force-pushed the dc/cli branch 3 times, most recently from ea1a526 to aede98a Compare December 19, 2023 21:55
@DanCardin
Copy link
Contributor Author

DanCardin commented Dec 20, 2023

Latest push should fix the 3.10 test issue, which seems like the remaining obvious PR problem.

Lmk if there's anything else you're specifically looking for (for example: docs? or leave that off/to you?) in this PR. (If so, dont bothering re-approving the CI since it'll need to run again anyways, and it shouldnt be necessary at this point 🤞)

Also like, commits. idk if you want me to squash everything or not


I plan for at least a couple of smaller follow-up PRs after this is merged. With stuff like --include and --exclude

@sambhav
Copy link
Collaborator

sambhav commented Dec 21, 2023

LGTM! Thanks for this! Let's follow up on subsequent PRs with additional features/changes.

@sambhav sambhav merged commit 86f2799 into argoproj-labs:main Dec 21, 2023
18 checks passed
@DanCardin DanCardin deleted the dc/cli branch December 21, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants