ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
21.04k stars 1.53k forks source link

Refactor `_config` module to use `pydantic` #3466

Open Viicos opened 10 months ago

Viicos commented 10 months ago

Following https://github.com/ManimCommunity/manim/pull/3440, we should refactor the _config module (and especially the ManimConfig class), preferably using pydantic to have builtin validation. Here are a few thoughts I had about this:

On this is tackled, we could think of adding support for other config formats (other than .cfg), e.g. TOML which is slowly becoming the standard. However, this introduces extra maintenance cost (especially on the documentation side), so this is still to be discussed.

I'll work on the first part and get an initial implementation this week, that can then be discussed for improvements

behackl commented 10 months ago

I've actually already started to implement a replacement of ManimConfig that uses pydantic -- but it is nowhere near complete, I got sidetracked when trying to add ManimColor as a custom type recognized by pydantic (which is perhaps not necessary; unclear). Given that you have more experience with pydantic, this will probably be less of a hurdle for you. :-)

I'd like to seize this opportunity and actually clean up the config though; I don't think the current version should just be ported 1:1. I'll add a comment to the list of current config options wherever I think that modifications should be considered.

    _OPTS = {
        "assets_dir",
        "background_color",
        "background_opacity",
        "custom_folders",  # this is confusing
        "disable_caching",
        "disable_caching_warning",  # not sure whether this is really useful
        "dry_run",
        "enable_wireframe",
        "ffmpeg_loglevel",
        "ffmpeg_executable",
        "format",
        "flush_cache",  # this should honestly not be a CLI flag of render, this should be a separate subcommand. (-> manim clean)
        "frame_height",
        "frame_rate",
        "frame_width",
        "frame_x_radius",
        "frame_y_radius",
        "from_animation_number",  # confusing name
        "images_dir",
        "input_file",
        "media_embed",  # -> jupyter_media_embed, or notebook_media_embed
        "media_width",  # as above. -> jupyter_media_width, or notebook_media_width
        "log_dir",
        "log_to_file",
        "max_files_cached",
        "media_dir",
        "movie_file_extension",  # why is this and format needed?
        "notify_outdated_version",
        "output_file",
        "partial_movie_dir",
        "pixel_height",
        "pixel_width",
        "plugins",
        "preview",
        "progress_bar",
        "quality",  # this should be a computed property to which one of several values of an Enum can be assigned to, this then sets resolution and frame rate. this could just return a tuple (resolution, frame_rate).
        "save_as_gif",  # remove
        "save_sections",
        "save_last_frame", 
        "save_pngs",  # remove
        "scene_names",  # does this really need to be in the config?
        "show_in_file_browser",
        "tex_dir",
        "tex_template",   # i am not a big fan of this and the next as config options. not sure whether we can justify removing them.
        "tex_template_file",
        "text_dir",
        "upto_animation_number",  # confusing name
        "renderer",
        "enable_gui",  # does this still work?
        "gui_location",  # does this still work?
        "use_projection_fill_shaders",   # remove
        "use_projection_stroke_shaders",  # remove
        "verbosity",
        "video_dir",
        "sections_dir",
        "fullscreen",  # does this work?
        "window_position",
        "window_size",
        "window_monitor",
        "write_all",  # not sure whether this should be a config option
        "write_to_movie",  # ideally, this should be removed (it is controlled by format). probably too much work for one PR.
        "zero_pad",  # should be renamed to make things clearer
        "force_window",  # does this work?
        "no_latex_cleanup",
    }

This is probably a project that should not be tackled in just one PR; perhaps it would make sense to work on a drop-in replacement of the current configuration class first before making any changes; it's hard for me to tell how much extra work these weird fields for the initial implementation really are.

Viicos commented 10 months ago

Thanks for making up the list! I think I'll first try to make the pydantic implementation, and we could indeed discuss how to handle the clean up (we'll probably have to see if we need to raise deprecation warnings in the first place for those that should be removed?)