-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[CLI] Add example command to CLI #1010
Conversation
…d-example-to-cli
To run a file in python without exec(f'import examples.{name}') (see |
@archibate Thanks for your insights! Apparently Per:
I'd like to get your insights, especially on whether we shall do a refactor in this PR or a seprate one. Right now I don't see any tests for the CLI, which makes the development around it a bit tricky. Ideally if we could switch to something like: commands = parser.add_subparsers(help='Available Taichi commands')
examples = commands.add_parser('examples', help='Interact with Taichi examples')
examples.add_argument(****) for most of the commands/flags/arguments, it will be much eaiser to unit-test them and it's less error-prone. I left |
Hi folks! This PR is ready for review, please take a look when you get a chance, thanks 😊 |
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.
Thank you! This is very helpful. LGTM in general except for a few minor issues.
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.
Thank you so much! Everything looks good to me now. After adding this line https://github.com/taichi-dev/taichi/pull/1010/files#diff-2eeaed663bd0d25b7e608891384b7298R19
Your CLI works when Taichi is installed as a Python package. I'll merge after CI passes!
Related issue = #775
Implementation plan and thoughts
ti example mpm88
work, with dead-simple invalid argument handling (ti example fake
). Leveraging the existing condition branches for simplicity.dev_install
docs as I setup development environment. (Please let me know if this makes the PR too convoluted, I could remove and cherry-pick it from a separate [doc] PR)[ ] Instead of using the long-windedout of scopeif-elif-else
, move theexample
subcommand to a more Pythonicargparse
based CLI. This will need insights from the core devs since it requires larger refactor, and might be ideal to implement in a separate PR.os.system()
, which is error-prone. I ended up usingrunpy
with absolute paths.[ ] Add tests for CLI forout of scopeexample
command.[ ] The order of returned set of available example names when it errors out is not guaranteed, this is more of an UX question than a technical problem. I'll leave it as it is for now.out of scopeti example
is now runnable from anywhere on the system, as long as the script is registered.Yak shaving
shutil
import forconvert
subcommand.Testing commands
[Click here for the format server]