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

Refactor _config module to use pydantic #3466

Open
Viicos opened this issue Nov 20, 2023 · 2 comments · May be fixed by #3473
Open

Refactor _config module to use pydantic #3466

Viicos opened this issue Nov 20, 2023 · 2 comments · May be fixed by #3473
Assignees
Labels
new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)

Comments

@Viicos
Copy link
Member

Viicos commented Nov 20, 2023

Following #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:

  • use pydantic-settings to make use of settings related features: creating a custom source to parse .cfg files (and be able to parse env vars as well?)
    • default.cfg will be removed in favor of model default values (+ Some comments were outdated in this file as well).
  • Refactor utils like make_logger, parse_theme, etc. Ideally, they should be tied to configparser.

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

@Viicos Viicos added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Nov 20, 2023
@behackl
Copy link
Member

behackl commented Nov 20, 2023

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
Copy link
Member Author

Viicos commented Nov 20, 2023

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?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)
Projects
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

2 participants