Skip to content

Refactored the Command Line Interface to use Click instead of Argparse #1013

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

Merged
merged 86 commits into from
Apr 2, 2021

Conversation

jsonvillanueva
Copy link
Member

@jsonvillanueva jsonvillanueva commented Feb 8, 2021

Motivation

Closes #452

This is a refactor which moves Manim away from argparse and towards click in order to allow future developers to easily add future subcommands, options, and arguments.

Changelog / Overview

This change breaks the CLI API to organize the structure of Manim Community's commands, options, and arguments.

To be more in line with POSIX compliant CLI conventions, options for commands are given BEFORE their arguments.
In Argparse: manim basic.py -p -ql
With Click: manim -p -ql basic.py

Although this is primarily a refactor and most of the common options are still there, some options have been added/removed. Use the manim command's -h, --help option, or simply run the command without providing options/arguments to view the help page with the full list of subcommands/options/arguments.

  • Added a --fps/--frame_rate option which allows for custom fps that don't have to be integer (i.e. 29.97, 23.98, etc.). Users no longer have to specify the FPS from within a config file. Additionally, the --webgl_renderer_fps option has been removed. Use --fps or --frame_rate instead.
  • Added a --renderer option which you can use to select your choice of renderer (e.g. --renderer=opengl). There are currently THREE renderers to choose from!
  • Removed the --background_color option. Reassigned the --background_color option's shorthand -c to --config_file.
  • Removed the --leave_progress_bars option. Use --progress_bars=leave instead.
  • Removed the deprecated render quality flags, in particular: -l, -m, -h, -k.
  • Removed the --sound option. It lost support long ago with the removal of SoX.

By default, the manim group calls the manim render subcommand through some decorator magic (i.e. manim -pql basic.py is a shorthand for manim render -pql basic.py). All of the subcommands and their options are contained within the manim/cli directory which is where the focus of the code should go. Most testcases that involve using the CLI had to be refactored as well to accomodate the new method of providing options before arguments ( arguments being the file/scene(s) ).

Testing Status

All test cases are currently pass on all OSs in our CI!

Further Comments

There will be some refactoring in future commits to this PR, particularly documentation of the functions. This is a successful first draft though.

The intent is to eventually redo the ManimConfig object as well using a similar decorator style approach to easily add config options with customized validation logic.

The new CLI structure is as follows:

manim [Defaults to: render*]
    |- options
|---render
    |- options
    |-file, scenes
|---cfg
    |---export
        |- options
    |---show
    |---write
        |-options
|---plugins
    |-options

If you're curious to see the new help messages without adding my remote:

manim -h

manim -h

Usage: manim [OPTIONS] COMMAND [ARGS]...

  Animation engine for explanatory math videos

Options:
  --version   Show the version and exit.
  -h, --help  Show this message and exit.

Commands:
  render*  Render SCENE(S) from the input FILE.
  cfg      Manages Manim configuration files.
  plugins  Manages Manim plugins.

  Made with <3 by Manim Community developers.

manim render -h

manim render -h

Usage: manim render [OPTIONS] [FILE] [SCENES]... COMMAND [ARGS]...

  Render SCENE(S) from the input FILE.

  FILE is the file path of the script.

  SCENES is an optional list of scenes in the file.

Options:
  Global options:
    -c, --config_file FILENAME    Specify the configuration file to use for
                                  render settings.

    --custom_folders              Use the folders defined in the
                                  [custom_folders] section of the config file
                                  to define the output folder structure.

    --disable_caching             Disable the use of the cache (still
                                  generates cache files).

    --flush_cache                 Remove cached partial movie files.
    --tex_template FILENAME       Specify a custom TeX template file.
    -v, --verbose [DEBUG|INFO|WARNING|ERROR|CRITICAL]
                                  Verbosity of CLI output. Changes ffmpeg log
                                  level unless 5+.

  Output options:
    -o, --output TEXT             Specify the filename(s) of the rendered
                                  scene(s).

    --media_dir PATH              Path to store rendered videos and latex.
    --log_dir PATH                Path to store render logs.
    --log_to_file                 Log terminal output to file  [default: True]
  Render Options:
    -n, --from_animation_number TEXT
                                  Start rendering from n_0 until n_1. If n_1
                                  is left unspecified, renders all scenes
                                  after n_0.

    -a, --write_all TEXT          Render all scenes in the input file.
    -f, --format [png|gif|mp4]
    -s, --save_last_frame
    -q, --quality [l|m|h|p|k]     Render quality at the follow resolution
                                  framerates, respectively: 854x480 30FPS,
                                  1280x720 30FPS, 1920x1080 60FPS, 2560x1440
                                  60FPS, 3840x2160 60FPS

    -r, --resolution TEXT         Resolution in (W,H) for when 16:9 aspect
                                  ratio isn't possible.

    --fps FLOAT                   Render at this frame rate.  [default: 30]
    --webgl_renderer PATH         Render scenes using the WebGL frontend.
                                  Requires a path to the WebGL frontend.

    -t, --transparent             Render scenes with alpha channel.
    -c, --background_color TEXT   Render scenes with background color.
                                  [default: #000000]

  Ease of access options:
    --progress_bar [display|leave|none]
                                  Display progress bars and/or keep them
                                  displayed.  [default: display]

    -p, --preview                 Preview the rendered file(s) in default
                                  player.

    -f, --show_in_file_browser    Show the output file in the file browser.
    --sound                       Play a success/failure sound.
  -h, --help                      Show this message and exit.

  Made with <3 by Manim Community developers.

manim cfg -h

manim cfg -h

Usage: manim cfg [OPTIONS] COMMAND [ARGS]...

  Manages Manim configuration files.

Options:
  -h, --help  Show this message and exit.

Commands:
  export
  show
  write

  Made with <3 by Manim Community developers.

manim cfg export -h

Usage: manim cfg export [OPTIONS]

Options:
  -d, --dir TEXT
  -h, --help      Show this message and exit.

manim cfg show -h

Usage: manim cfg show [OPTIONS]

Options:
  -h, --help  Show this message and exit

manim cfg write -h

Usage: manim cfg write [OPTIONS]

Options:
  -l, --level [user|cwd]  Specify if this config is for user or the working
                          directory.

  -o, --open
  -h, --help              Show this message and exit.

manim plugins -h

manim plugins -h

Usage: manim plugins [OPTIONS]

  Manages Manim plugins.

Options:
  -l, --list  List available plugins
  -h, --help  Show this message and exit.

  Made with <3 by Manim Community developers

Acknowledgements

Reviewer Checklist

  • Newly added functions/classes are either private or have a docstring
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings
  • The oneline summary has been included in the wiki

@jsonvillanueva
Copy link
Member Author

I just realized something:
before this pr in jupyter notebooks:

%%manim   -s  -v WARNING  ABCScene  # does NOT work
%%manim  ABCScene  -s  -v WARNING  # works

after this pr in jupyter notebooks:

%%manim   -s  -v WARNING  ABCScene  # works
%%manim  ABCScene  -s  -v WARNING # # does NOT work

@kolibril13 This is intended as this PR is a breaking change -- there's not much we can do about this other than give a deprecation warning since click standardizes our CLI to be POSIX compliant. It's not in the realm of possibility to add reverse compatibility for the the old Argparse way. The deprecation warning is already given in the CLI:
image

Note: I still think this PR can be merged for v0.5.0, just waiting on @naveen521kk's review. I believe it's stable enough. I'm just wondering whether it'd be better to introduce it at the beginning of the next release cycle for v0.6.0, or towards the end for v0.5.0

@naveen521kk
Copy link
Member

I'm not exactly sure how to get click to output the traceback as before.

No problem we can fix it later. Please open an issue.

Based on the poll from earlier, most people wanted the --background_color flag removed. As such, I made the -c option for config_file but didn't entirely remove the background_color option just yet.

Looks like changelog is missing it. Can you update it?

@jsonvillanueva
Copy link
Member Author

Updated and opened #1207

@kolibril13
Copy link
Member

kolibril13 commented Mar 31, 2021

@kolibril13 This is intended as this PR is a breaking change -- there's not much we can do about this other than give a deprecation warning since click standardizes our CLI to be POSIX compliant

Alright, I am definitely fine with merging it then also before 0.5.0.
The one thing I wanted to say is that there might be the %%manim -p -pql - syntax soon for the jupyter notebooks,
so if people want to translate their 0.4.0 notebooks to the new version, this might be convenient to only remove the scene name and add a - in the end. But also not so important to hold this pr back.

@jsonvillanueva
Copy link
Member Author

jsonvillanueva commented Mar 31, 2021

The one thing I wanted to say is that there might be the %%manim -p -pql - syntax soon for the jupyter notebooks,
so if people want to translate their 0.4.0 notebooks to the new version, this might be convenient to only remove the scene name and add a - in the end. But also not so important to hold this pr back.

@kolibril13
I actually already implemented this using something similar to the - syntax. See bfe3f1e, ipython_magic.py:

result = runner.invoke(main, args, input=cell)

I discussed this very briefly with @behackl, but it didn't fly because there can be code blocks with more than one Scene? Or something of that nature. But this is using the code block directly as stdin and passing it to be rendered. I only use the second instance of running the command to get the output file's directory so that it can be displayed. IMO, this is a cleaner way to do this, but it'd be good if the config object could be returned so it didn't have to be run a second time just for the output file path.

@naveen521kk
Copy link
Member

naveen521kk commented Mar 31, 2021

Looks like everything from #1013 (comment) is fixed. Also, I couldn't find the changelog of --renderer flag introduced. Also, is renderer introduces in Config? Either we should add it now or open an issue. Or is it missing in the docs? https://manimce--1013.org.readthedocs.build/en/1013/tutorials/configuration.html#a-list-of-all-config-options

I will do a review soon.

@jsonvillanueva
Copy link
Member Author

jsonvillanueva commented Mar 31, 2021

Looks like everything from #1013 (comment) is fixed. Also, I couldn't find the changelog of --renderer flag introduced. Also, is renderer introduces in Config? Either we should add it now or open an issue.

I will do a review soon.

Renderer is introduced in ManimConfig and is also a new available option. I'll write something for it right now.
Please feel free to edit my main message if you find something else wasn't in the changelog. Or if you just have stylistic stuff/formatting that helps too

Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

lgtm

@kolibril13
Copy link
Member

there can be code blocks with more than one Scene?

Good point. However, I think having more than one Scene in one cell in jupyter is very unlikely because, in my opinion, the whole point of using jupyter is the ability to use multiple cells for multiple scenes.
But you are right, this might happen that there are multiple scenes.
In this case, the user could get a warning "Warning: More than one scene in this cell detected. Only first scene will be rendered."

@jsonvillanueva
Copy link
Member Author

the whole point of using jupyter is the ability to use multiple cells for multiple scenes.

That's what I was thinking when I went ahead and implemented it -- every example jupyter notebook I've seen only puts one scene in the cell. I haven't actually seen multiple Scenes in a cell, or tried it myself tbh. I'm genuinely unsure if manim currently even supports multiple Scenes in jupyter-lab.

Also, I think it would be possible to use stdin ending with - to render multiple scenes but I don't think this should be implemented in ipython_magic.py. It should be implemented where we do the check for - which is in module_ops.py: get_module

@behackl
Copy link
Member

behackl commented Mar 31, 2021

It seems like we could still include this in v0.5.0. I'm somewhat undecided whether we should do so, or whether we should merge this as the first PR after the release.

I'm slightly leaning towards including it in v0.5.0 (and in the worst case of something fundamental being broken quickly pushing out a bugfix release v0.5.1).

@jsonvillanueva
Copy link
Member Author

jsonvillanueva commented Mar 31, 2021

I'm slightly leaning towards including it in v0.5.0 (and in the worst case of something fundamental being broken quickly pushing out a bugfix release v0.5.1).

The thought of doing a patch release hadn't crossed my mind -- especially since most of our releases have had some fairly big PRs (webgl, svg, opengl, etc.) -- but this sounds good to me. I've merged upstream master, feel free to merge when ready -- or after the release if you change your mind. I do have some stuff I'd still like to integrate but these can (and should) be done in a future PR.

janluke and others added 2 commits March 31, 2021 17:48
1) There's a new file for each option group
2) render is now a cloup.Command, not a Group

Fixed issue when an animation is cached, manim can't merge the partial movie files. (ManimCommunity#1192)

* fixed issue

* fixed tests

* Update manim/renderer/cairo_renderer.py

Co-authored-by: Darylgolden <darylgolden@gmail.com>

* added tests

* imrpoved test

* fixed logic

* added new test

* check if the file has been outputed

* added test when caching is enabled

* fixed tests on windows

* black

* Update manim/renderer/cairo_renderer.py

Co-authored-by: Naveen M K <naveen@syrusdark.website>

* Update tests/assert_utils.py

Co-authored-by: Naveen M K <naveen@syrusdark.website>

Co-authored-by: KingWampy <9156604+WampyCakes@users.noreply.github.com>
Co-authored-by: Darylgolden <darylgolden@gmail.com>
Co-authored-by: Naveen M K <naveen@syrusdark.website>

Added :ref_methods: to the manim directive (ManimCommunity#1209)

* fix manim_directive for methods

* added ref_methods to Angle example

* black

* added new ref_methods references

* sort out ref_functions vs ref_methods in examples.rst

Co-authored-by: Jason Villanueva <a@jsonvillanueva.com>
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>

Fixed issue when an animation is cached, manim can't merge the partial movie files. (ManimCommunity#1192)

* fixed issue

* fixed tests

* Update manim/renderer/cairo_renderer.py

Co-authored-by: Darylgolden <darylgolden@gmail.com>

* added tests

* imrpoved test

* fixed logic

* added new test

* check if the file has been outputed

* added test when caching is enabled

* fixed tests on windows

* black

* Update manim/renderer/cairo_renderer.py

Co-authored-by: Naveen M K <naveen@syrusdark.website>

* Update tests/assert_utils.py

Co-authored-by: Naveen M K <naveen@syrusdark.website>

Co-authored-by: KingWampy <9156604+WampyCakes@users.noreply.github.com>
Co-authored-by: Darylgolden <darylgolden@gmail.com>
Co-authored-by: Naveen M K <naveen@syrusdark.website>

Added :ref_methods: to the manim directive (ManimCommunity#1209)

* fix manim_directive for methods

* added ref_methods to Angle example

* black

* added new ref_methods references

* sort out ref_functions vs ref_methods in examples.rst

Co-authored-by: Jason Villanueva <a@jsonvillanueva.com>
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>

Fixed unnecessary args dict
@jsonvillanueva jsonvillanueva linked an issue Apr 1, 2021 that may be closed by this pull request
@behackl behackl modified the milestones: Release v0.5.0, Release v0.6.0 Apr 2, 2021
@jsonvillanueva jsonvillanueva merged commit a87bb28 into ManimCommunity:master Apr 2, 2021
@jsonvillanueva jsonvillanueva deleted the click branch April 2, 2021 07:08
@behackl behackl mentioned this pull request Apr 2, 2021
1 task
@kolibril13
Copy link
Member

I just realized that the current help message does not show anymore the options for manim, can we maybe bring this back?
image

@kilacoda-old
Copy link
Contributor

manim render -h

@kolibril13
Copy link
Member

Maybe we can add an extra sentence here, e.g. for more help, type manim renderer -h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes This PR introduces breaking changes highlight For contributions that should be highlighted explicitly in the next changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove background_color commandline flag Specify FPS from commandline Using Cleo instead of Argparse
10 participants