Refactoring the config system

[leotrs]

Yep, the config system strikes again. I'll try to keep it short this time. The main point is that, while the current config system works fairly well, it's quickly become a mess and difficult to maintain. For example, here are just two problems with it:

1. The exported dictionaries config and camera_config are ambiguous -- Usage of config vs camera_config and separating them. #283
2. You can't set the config when running from a file (for example when testing or making documentation) -- see Manually selecting a manim.cfg when NOT running from ClI #293
3. file_writer_config, cameraconfig_, config are confusing, and sometimes read/parse the same cfg files twice -- see REFACTOR : _run_config() should not return file_writer_config. #443

The plan

My plan for refactoring is the following, modified from this comment:

I want to refactor the config system so that the order of events would be the following:

1. A script executes import manim, which in turn executes manim/__init__.py. This can be a user scene script or our own manim/__main__.py.
2. In __init__.py, the first line of code is import config which executes config/__init__.py.
3. config/__init__.py first executes config/config.py.
4. In config.py, as much as possible of the config dict should be defined. In particular, all config files must be read and parsed. Also, config.py also parses the sections of the configuration corresponding to the logger, and exports them as well. All of config, camera_config, and file_writer_config are defined and exported. NOTE the logger is NOT set here.
5. Back in config/__init__.py, the next line it runs executes config/logger.py.
6. config/logger.py takes the config options exported from config/config.py and uses them to set the logger. NOTE this eliminates the need to execute _run_config twice.
7. Back in config/__init__.py, everything exported by config/config.py and config/logger.py is exported up to the top-level package. After this, the config subpackage should never really have to be used again (other than for importing config/logger). config/__init__.py now returns execution to the top-level __init__.py.
8. Back in __init__.py, here comes the split.

• If the original script is a user script, then we are done and execution is returned to the user. They can then go about defining their Scene classes and ultimately rendering them.
• If the original script was __main__ because manim has been called from the command line, __init__.py returns execution to __main__.main(). Now, this function must deal with parsing command line flags and then updating the global config dict. This can easily be done by implementing e.g. manim.config.parse_cli(args). Note this will remove the problem we've had in the past where config.py had to determine whether it had been called from the command line (i.e. the hacky _from_command_line function). Once the CLI flags have been parsed and the config updated, __main__.main() simply decides what to execute next, depending on the subcommand used. Thus, a big chunk of cfg_subcommands will be ported into __main__.py.

Associated PRs

This is a fairly major overhaul so I'll try to keep the PRs short and self-contained so that they are easy to review.

• Refactor config #340 (merged): add the tempconfig context manager for testing all of the upcoming changes. Also make Camera read the config at instance construction, not at class definition.

• Create config subpackage #376 (merged): creates a new subpackage for all the stuff related to the config system.

• Stop using config at import #397 (merged): makes all modules use the config dict as little as possible during library initialization.

• Refactor config subpkg #543 (merged): refactors the config subpackage so that only config/__init__.py defines and exports global variables, whereas all other files just contain utilities.

• Replace the global config dict with new class ManimConfig #620 (merged): replaces the global config with a custom class ManimConfig.

[leotrs]

Ok @ManimCommunity/core so I've been thinking hard and long about the next steps for the config system. Right now everything seems to work fine, but it's all haphazardly put together, and there are some missing features. For example:

• We would like to be able to choose the config file on the fly, not necessarily choose it from the command line (Custom config file also when not run from CLI #219). This is currently impossible.
• There are some individual options that can be changed on the fly, using tempconfig context manager. Some are impossible, or don't even exist. For example, config['transparent'] doesn't exist. Let's say I want to set transparency mode on from my script. I cannot do this. Let's say I want to know if the user passed the --transparent flag via the command line. I cannot do this unless I parse sys.argv. Messy.
• The current dict is not self-consistent. In the example above, we could just define a new key in the dict, config['transparent'], and set it to whatever the user passed via CLI. But then let's say I change config['background_opacity'] = 1. The key config['transparent'] will be True, even if the background is actually opaque. Inconsistent.
• If you wanted to know what exactly does the --transparent flag do, you would have to go to two different places: first, default.cfg has a [transparent] section, and second, config.py actually uses it. Except it doesn't, you have to actually check config_utils.py, and then navigate the spaghetti code that is _parse_file_writer_config. (BTW why is transparent part of file writer config and not camera config? Weird.) Documenting the source of each config option as well as the effect is a nightmare, as config is currently just a dict, and documenting each individual key of a dict is tricky.

As you can see, there are many problems. I believe I have a way to fix all of these in a way that is (mostly) backwards compatible. Basically, we define a new class that behaves like a dict but also contains all of the information that keeps it self-consistent. I believe this is the way to go, but I wanted everyone's opinions before implementing.

Here is how that would look like:

from collections.abc import MutableMapping

class ManimConfigDict(MutableMapping):
    ALL_OPTS = ['pixel_width', 'pixel_height', 'aspect_ratio']
    def __init__(self):
        self.__dict__ = {k: None for k in ALL_OPTS}

Remember setting the __dict__ is equivalent to setting attributes, so if you define c = ManimConfigDict(), then c.pixel_width, c.pixel_height, and c.aspect_ratio are all valid.

What does this ManimConfigDict do? It behaves like a dict:

    def __getitem__(self, key):         # c['pixel_width'] returns c.pixel_width
        return self.__dict__[key]      
    def __iter__(self):                 # required by MutableMapping
        return iter(self.__dict__)
    def __len__(self):                  # required by MutableMapping
        return len(self.__dict__) 

This means that c['pixel_width'] just returns c.pixel_width, so this is entirely backwards compatible, as we don't have to change any code that already uses the dict syntax.

But also we don't want anyone to delete any keys, so

    def __delitem__(self, key):
        raise NotImplementedError('Deleting keys from ManimConfigDict is not allowed.')

Now here is the fun part. What happens if you set c['pixel_width'] = 10? You can totally do that, but now you have screwed the aspect ratio because you forgot to change c['pixel_height'] as well. Well, this class keeps self-consistent, roughly as follows:

    def __setitem__(self, key, value):
        self.__dict__[key] = value      # this will in turn call the properties defined later

    @property
    def pixel_width(self):
        return self.__dict__['pixel_width']

    @pixel_width.setter
    def pixel_width(self, val):
        # keep aspect ratio!
        self.__dict__['pixel_width'] = val
        self.__dict__['pixel_height'] = val / self.aspect_ratio

So now every time that you change the width, the height keeps the correct aspect. And you can do the same for the height. And you can even change the aspect ratio itself and make the width/height keep consistent.

So let's see how this solves the problems above:

• Choose the config file on the fly: just read a .cfg file and set all of the attributes of c that it defines. The MutableMapping class even defines the update method for you, so you can just call c.update(dict_with_options_read_from_file) and be done.

• config['transparent'] doesn't exist, and if it did, it would not keep consistent: just create a transparent property in ManimConfigDict and make it so that changing config['transparent'] changes config['background_opacity'], and vice versa.

• What does the --transparent flag do, exactly? Just go to the documentation for the ManimConfigDict.transparent property. It's even online, it will have extensive examples and be type annotated.

• We want it backwards compatible: ManimConfigDict just behaves like a dict, so it doesn't break legacy code. In fact, if we make this change and your scene breaks, it's probably because it wasn't keeping self-consistent. A change that actually shines a spotlight on existing bugs is a good thing, I think. (And this goes for users as well as the codebase itself.)

Because the config system is its own subpackage (#376), we just have to change one or two files in the codebase. All other files can keep using config['pixel_width'] without problem.

What do you think? Did I miss anything? Are there better ways of implementing this? Please let me know.

Please leave me an emoji here to make sure we have consensus before I go about implementing this.

[yoshiask]

Remember setting the dict is equivalent to setting attributes, so if you define c = ManimConfigDict(), then c.pixel_width, c.pixel_height, and c.aspect_ratio are all valid.

My concern with this is that we have a similar issue that the CONFIG system had/has. Because the attributes are added at runtime, will an IDE for example be able to recognize that those attributes are there?

[leotrs]

Fair. I can manually add each config option as an attribute, set them to None, and then replace them with the corresponding property dynamically afterward. I think that'd work?

[huguesdevimeux]

First of all, thank you for this post and taking time to do this. Everything is vert good to me.

Question tho : What if one wants to mofify the config during the process ? for example, file_writer_config['disable_caching'} is enabled when the code comes from stdlin, how could we do this after this ?
Sorry if it was said.

[leotrs]

Question tho : What if one wants to mofify the config during the process ? for example, file_writer_config['disable_caching'} is enabled when the code comes from stdlin, how could we do this after this ?

I'm not sure I understand the question, but this sounds like exactly what tempconfig is designed to do. Otherwise, the ManimConfigDict.update() method can change the config permanently.