Configparser setup: figuring out config.py, constants.py, dirs.py, CLI arguments, and more [WIP]

[leotrs]

Ok y'all, this is a doozy. I've been working on this and there's a few things that we need to decide as an organization. I'm going to try to make my points clearly and I'll bold the points at which I need y'all's input.

Rationale

The current repo is very badly disorganized and needs a good cleanup. One of the sources of this is the fact that there are different ways of manipulating what manim does as a whole:

1. constants.py sets some global variables (most of which are actually constant, but some of which are not).
2. config.py parses the CLI arguments, uses it to set camera_config among other things. It also initializes directories.
3. HOWEVER, there is also a dirs.py file that allowed you to set default output directories. config.py reads that in as well as the CLI arguments.

As a result, I think it's quite the hurdle for a user to figure out where to get or set the value of a particular variable that determines how manim works. If I want to change a directory, should I use dirs.py, or a CLI argument? Which has precedence over the other? If I want to change the resolution, should I do it in constants.py, by CLI argument, or in the CONFIG of my Scene objects?

The main purpose of this PR is to provide a single default source of global truth for configuring how manim works. (And, in the event that there is more than one possibility to set the value of a particular variable, the precedence between those possibilities should be very clear and documented.)

The proposal

The main way to do this is as follows. This PR makes it so that there is one single place to write/determine/set how manim works, and one single place to read/check/get those configurations once they are in place.

• Write: The user can now use an optional manim.cfg file to tweak the default behavior. Note this is a file that the user can have on their machine without touching the manim codebase at all. We will also ship a default config file so the user doesn't need to have such a file, but can have one if they want it.
• Read: once all configuration variables, directories, and CLI arguments have been parsed/processed, there is one single place where all of the configuration lives in for the rest of the execution: the global config dictionary that can now be imported by doing from .config import config (if within the library codebase) or by from manim import config (if importing the library as end user).

The constants.py file still exists, but as much as possible, it should not contain configuration variables, only constants. (more on this later).

How it works

I want to explain the main workflow of how this works. For this, note that there are two different ways of using manim: importing it as a library (to have access to Scene, Mobjects and so on), and as a command line script (to actually render a video).

If being imported as a library, this is what's going to happen. The user will type from manim import * at the top of their file. This executes the contents of __init__.py, which in turn imports/executes config.py before anything else. config.py defines and exports the config dictionary by parsing the manim.cfg file. This config will now contain all configuration needed for the end user (and indeed for internal purposes as well). Finally, config.py will also setup some directories (more on this later). (Note that this is a very odd way of using manim, like for example if you open a python interpreter and type import manim. Importantly, this will also be the case during testing. In most cases, manim will be used in the other way, in the next paragraph.)

If being used from the command line, for example as python -m manim ... or as manim ... or as manimcm ..., the file that gets executed first is __main__.py. This file also imports/executes config.py as the first order of business. In this case, config.py will recognize it's being used from the command line and, after parsing manim.cfg first, it will also parse the CLI arguments. The CLI arguments will override the default settings defined in manim.cfg. At the end, the global config dictionary will contain all configuration that correctly corresponds to the defaults in the .cfg file as well as the CLI arguments.

The result

When manim is being used as a library, there is no visible change other than the fact that some imports of the type from manim.constants import SOME_VAR and then using SOME_VAR will become import manim.config as config and then using config['SOME_VAR'], because some variables (that weren't constants) have been moved from constants.py into the config dictionary. More on this later (see discussion points).

When manim is being used from the command line to actually generate scenes, the changes are as follows. All the command line arguments are still in place. BUT, now the user may set the defaults by using their own manim.cfg file. For example, the -p (or --preview) flag is turned off by default so if the user wants the movie to play after it's rendered, currently they need to always specify -p. After this PR, the user can create a new manim.cfg file with the line PREVIEW = TRUE. If they do so, running manim without the -p flag will still show the video once it's rendered. This has the advantage that if a user needs a particularly long CLI command with lots of flags to render their scenes, they can just dump all of those flags into the manim.cfg file and just run manim myfile.py. The manim.cfg file currently allows to set default values for each command line argument.

The discussion points

I'm just going to dump a bunch of issues here in no particular order. I'll do my best to be brief and to point exactly what needs to be answered/discussed/decided in bold.

1. The main problem here is that we are still conflating constants along with configuration variables, in the following way. The PIXEL_WIDTH (previously called DEFAULT_PIXEL_WIDTH) variable can be set in command line via the -l, -m, -e, -r flags. So it is not a constant, and it should live inside config, not in constants.py. This PR makes it so. Now, there are other values, for example FRAME_WIDTH, that directly depend on the value of PIXEL_WIDTH, thus they are also not constant. HOWEVER, the configparser library that parses .cfg files does not support arithmetic operations. So right now there is no easy way to only set PIXEL_WIDTH once and have FRAME_WIDTH and related variables update their values as well. Right now, in this PR this is being done in the constants.py file: it is loading the config dictionary, which contains PIXEL_WIDTH and defines the other variables accordingly. I very much don't think this should be merged as is, but I wanted to ask what is the best way to solve this, knowing that this cannot happen in the .cfg file? My suggestion is that this happens inside config.py, with a special function that defines those variables such as FRAME_WIDTH once PIXEL_WIDTH is set. However, this means that the user can set only the latter and the former will always be tied to it in the same hard-coded way. EDIT: I think a good example to fix this issue is to consider the question, where do we define TOP? Currently, it is defined in constants.py, with the line TOP = FRAME_Y_RADIUS * UP. Now, I think UP is a true constant and it should stay in constants.py. But FRAME_Y_RADIUS depends on FRAME_HEIGHT, which is not constant and is set by CLI argument in config.py. If we adhere to the strict view that only true constants must remain in constants.py, then TOP must go elsewhere.

2. Due to the above (some configuration variables are being loaded into constants.py), I converted all configs and constants to ALL_CAPS. Accordingly, the CONFIG dictionary of Scenes and Mobjects were also turned to ALL_CAPS. I think this is ugly as hell and hope this changes in the future. Fixing the issue of constants vs config variables should make this go away.

3. For the problem of importing config.py before anything else, see Relative imports #26. I think we are in the clear here.

4. The current version does not work with python -m manim ... (see definition of prog in config.py). This is because it is actually very difficult to determine if the library is being imported or run from the command line (at least from the config.py file). I need suggestions for how to fix this.

5. This is a pretty big change so if there is anything that you can think of, please bring it up here.

6. Currently, all of the keys in SceneFileWriter.CONFIG are keys of the global config dict. Does SceneFileWriter really need a copy of each? Pinging @PgBiel here who is working on attrs.

7. Yes, this has been tested and renders scenes well. I have by no means ran an exhaustive test suite of all constants/configs/CLI flags yet.

List of changes

I'll list some of the minor changes made that are not big-picture issues. Please comment on these too if you have anything to say.

1. start_at_animation_number is now FROM_ANIMATION_NUMBER. Similarly, end_at_animation_number is now UPTO_ANIMATION_NUMBER. These are shorter and have the same meaning. (I don't like the ALL_CAPS, but that's a different issue touched upon above.)
2. dirs.py has been eliminated. Instead of dirs.video_dir, now you have to do config['VIDEO_DIR']. This will require changing from dirs import video_dir to from config import config and using config['VIDEO_DIR']. If we don't want to have to load the whole global config just for the one directory name, we can package the directories in a separate dict, also defined in config.py and load it as from config import dirs.
3. input_file_path has changed to INPUT_FILE, and file_name has changed to OUTPUT_FILE. The original names were just confusing.
4. The default configuration file is now packaged with the rest of the code, as seen in in setup.py.
5. DEFAULT_PIXEL_WIDTH, DEFAULT_PIXEL_HEIGHT, and DEFAULT_FRAME_RATE have been changed to PIXEL_WIDTH, PIXEL_HEIGHT, and FRAME_RATE. The reason is that once the config is read and the CLI arguments parsed, they are no longer a 'default' value, but possibly a value set by the user, so the names were misleading. The new names are also shorter.
6. PRODUCTION_QUALITY_CAMERA_CONFIG is now just the default config['CAMERA_CONFIG']. Also, note that config['CAMERA_CONFIG'] is changed by the *-quality flags (low-quality, medium-quality, high-quality) and by the --resolution flag.
7. The scene_names CLI argument is now --scene_names for easier parsing.
8. Changed open_video_upon_completion to PREVIEW so it has the same name as its corresponding CLI flag. It is also way shorter.
9. Removed the ignore_waits option which was never used and was set to the value of args.preview.
10. Most CLI flags have been changed to have no default value. For example, -p was by default False, but will now default to None. This is done that we can easily determine whether the user actually specified this flag by comparing it to None. If the flag is None, we read the default value from the manim.cfg file instead. Importantly, this is a subtle change in the code (see comments inside _parse_cli() in config.py if you are interested.
11. The way everything is setup right now, passing only the -s flag will also write a movie (since WRITE_TO_MOVIE is True by default in manim.cfg). See -w flag enabled by default #41.

List of standing or future issues

While working on this, I realized there is a number of other things that I would also like to change, but I'm not sure if they should be part of this PR or a separate one.

1. I think there are a bunch of variables/constants that have DEFAULT_ in the name but that should not have it.
2. The output directories are being created during initialization (when config.py is read). I think the directories should be created at the last possible minute. If I run manim and it crashes before writing the scene, the directories will be created but empty.

PS

Mostly for my sanity: this interacts with #65, #81, #77, #151 so I'll get back to those when this is merged.

[leotrs]

After more thought, I have a proposal that would fix points 1, 2, and 3 under "The Discussion Points" above.

In the original code base, config.py sets a different dictionary for camera_config, file_writer_config, and the rest of configurations are put in constants.py. In the current PR, I put the camera_config and file_writer_config as sub-dictionaries of the new config dictionary. I was being a bit conservative about this, but now I'm proposing that we make full use of those.

I propose that we leave inside constants.py only those variables X such that the answer to the following question is "no": Would the user expect X to change from project to project? If the user has two entirely different projects for which they are using manim, and they would expect X to have the exact same value in both projects, then X should go in constants.py, and everything else should go in the config dict. For example, for X=PIXEL_WIDTH the answer is "yes" (as the user may be using different frame sizes for different projects), and therefore it should go in config. For DEFAULT_MOBJECT_TO_EDGE_BUFFER the answer is "no" because even if the user needs buffers of different sizes for their two projects, this variable defines the default buffer, not the buffer that the user will need every time. A true default should be the same across projects, so this goes in constants.py. This solves 1) above because both PIXEL_WIDTH and TOP could change from project to project, as they both depend on the frame size and its geometry.

Another reason why I was reluctant to put TOP inside the config dict is because it would be awkward to have to do config.top. However, this is where the camera_config comes in handy. I suggest that we put all of the parameters relating to the geometry of the frame inside a frame_config global object, or it could even be named frame, and be imported as from config import frame. Then the user can just use frame.top (or frame['top'] if it's a dict). I think frame.width is much more pleasing than importing PIXEL_WIDTH into the local scope, as it's currently done by the use of from manim import *.

So, to sum up (and I do apologize for the literal walls of text in this PR), I'm suggesting that on top of the current PR, we take out (many) more variables out of constants.py into different objects/dicts that group them semantically. Concretely, I propose that we use the existing camera_config but expand it to contain all variables relating to the geometry of the frame/scene (and perhaps rename it to just frame), and use file_writer_config to contain all of the variables relating to writing files. In particular, we should move all of TEX_USE_CTEX, VIDEO_DIR, MEDIA_DIR to the file_writer_config (as the user could expect it to change from one project to another).

Please let me know if this makes sense, as this will involve quite a bit more refactoring and it will most likely NOT be backward-compatible.

EDIT: on the topic of backward-compatibility, we could easily implement this as well as keep the old constants/configs and emit a DeprecationWarning every time they are used!

[PgBiel]

My remarks so far before I go to sleep

HOWEVER, the configparser library that parses .cfg files does not support arithmetic operations.

Alternatives:

1. Allow eval() of inputs;
2. Have the config file be read as a .py file (and, therefore, require this format). Could be better for customizability and a few other factors, but... this idea is debatable

2. Due to the above (some configuration variables are being loaded into constants.py), I converted all configs and constants to ALL_CAPS. Accordingly, the CONFIG dictionary of Scenes and Mobjects were also turned to ALL_CAPS. I think this is ugly as hell and hope this changes in the future. Fixing the issue of constants vs config variables should make this go away.

Please, please, please have the config params be in lower (snake) case...

6. Currently, all of the keys in SceneFileWriter.CONFIG are keys of the global config dict. Does SceneFileWriter really need a copy of each? Pinging @PgBiel here who is working on attrs.

To be used with attributes yes. However, we do not need that, of course. We can just have the config dict be stored in some shape-free dictionary or something... This is, if the config types and whatnot are specified somewhere else (E.g. in some sort of Config class or w/e). Otherwise, we should use attributes in our favor, as they allow us to document each type and name.

[leotrs]

@PgBiel

Allow eval() of inputs;

I think eval() should be our last resort.

Have the config file be read as a .py file (and, therefore, require this format). Could be better for customizability and a few other factors, but... this idea is debatable

Yes, there are libraries whose configuration files are just plain .py files. However I'm still not sure we need the full power of that yet.

Please, please, please have the config params be in lower (snake) case...

Yes! My latest comment would achieve this, but it comes with the price of breaking changes (or DeprecationWarnings).

[Aathish04]

Have the config file be read as a .py file (and, therefore, require this format). Could be better for customizability and a few other factors, but... this idea is debatable

Yes, there are libraries whose configuration files are just plain .py files. However I'm still not sure we need the full power of that yet.

I really like the idea of using .py files for custom configuration. It offers a lot of space to expand if need be and could be used by any extensions to manim as well.

[leotrs]

Extensions/addons are actually a compelling argument.

In that case, the structure would probably look like this:

1. have one constants.py file (with ALL_CAPS, truly constant values),
2. have one master config.py which contains all the logic for default configuration,
3. before returning, the config.py could also load other user-defined or addon-defined configuration files to override the defafults.
4. before returning, the config.py could also parse CLI arguments, if called from the command line, to override the defaults.

This seems achievable. However, we still have the problem that it is very difficult to detect when manim has been called from the command line (see discussion point 4 in the OP).

BTW, the "normal" way of having separate behavior when called from the command line is to put all of that logic inside the __main__.py function, which is never imported by __init__.py the way that config.py is. However, our CLI arguments are changing the global configuration, so we need to parse CLI arguments, if they exist, at the time of importing config.py.

[huguesdevimeux]

I really like the idea of using .py files for custom configuration. It offers a lot of space to expand if need be and could be used by any extensions to manim as well.

IMO it makes more sense if it's a .cfg file. If we want the user to be able to create his own config without touching the manim codebase at all, a .py is a bad idea as it will certainly confuse some people who won't understand what file to run - why - where.
I mean, put yourself in the mind of a python beginner; you would way better understand the config system with a .cfg than with a .py.

To solve the problem of the arithmetic operations,

1. have one master config.py which contains all the logic for default configuration,
2. before returning, the config.py could also load other user-defined or addon-defined configuration files to override the defaults.
3. before returning, the config.py could also parse CLI arguments, if called from the command line, to override the defaults.

That is the best idea. In addition, we wouldn't be too far from the actual functioning.
And a config.py will be the place to handle when arguments miss (i.e not specified in the args or in the config file, due to a user mistake)

[leotrs]

On the problem of moving the arithmetic operations from constants.py to config.py:

1. It should leave constants.py not because of the arithmetic, but because it depends on the user defined value FRAME_WIDTH.
2. I'm more and more convinced it should just go on config.py. After all, this is currently the file that parses user input (through CLI arguments or through the .cfg file).
3. My initial hesitation was because the config.py is already a bit bloated as it is, and I didn't want to add even more specific logic. HOWEVER, I think the fix is to have a FrameConfig class in config.py, which processes/parses/sets all the frame and camera configurations. That way all this logic will be encapsulated in a single class. Again, only one source of truth, as opposed to this logic being strewn around the config.py file. Then, config.py can export a FrameConfig object, or just the __dict__ of it, or even just put it in the global config['camera_config'] as it currently is.

[leotrs]

And a config.py will be the place to handle when arguments miss (i.e not specified in the args or in the config file, due to a user mistake)

Small clarification: this will never happen, because there will always be a .cfg file shipped with manim. If the user doesn't specify a CLI argument or a .cfg file, there will be a library-wide .cfg file to set defaults.

[leotrs]

OK I just figured out how to solve problem 4) above. It's hacky, but it will work.

Also, a lot of the constant/config variables are only ever used by scene_file_writer.py. I'm thinking another thing we can do is group all of those into a single dict that is then just passed directly to scene_file_writer.py instead of putting them into the global dict. No need to expose them to the user since the user never really needs them.

[leotrs]

OK I heard y'all loud and clear and just pushed some changes. Let me summarize:

1. After the latest commit, even more variables have been taken from constants.py and put into config.py. Now constants only contains variables that will never change. Now constants are ALL_CAPS while configuration variables (the keys in the global config dict are snake_case).

2. I realized that the vast majority of config variables were related to output/writing/files and only ever used internally. I can't think of a reason why the user would ever touch those variables when writing a Scene. Therefore, I split the config variables into two global dictionaries: config, meant to store stuff the user will use, and file_writer_config, meant to be used internally by manim when writing output. Both are global dicts so the user can access the latter too if required, but now the dict manim.config contains only stuff that the user is more likely to need when writing Scenes. I think file_writer_config is an ugly name though, and I'm open to alternatives.

3. The file extract_scene.py has been copy-pasted onto __main__.py, since that was basically what it was. All other things that __main__.py was doing other than execute extract_scene.main have moved to config.py.

4. The CONFIG dictionaries of a couple of classes have been removed, since they were only ever using the variables found in the global file_writer_config and the instances of those classes never needed their own copy. This should affect the move to attr/dataclasses @PgBiel.

5. This all now works when being called as manim ..., manimcm ..., or python -m manim .... The latter is a hack, but it works @eulertour.

6. I tested this and it seems to be working!

7. I think this is ready for a more thorough review, so I'm removing the "draft" label.

[eulertour]

The problem with the slow rendering is caused because the Scenes are being initialized incorrectly.

manim/manim/__main__.py

Line 155 in b660303

scene = SceneClass()

Compare to master:

manim/manim/extract_scene.py

Line 176 in 4b6492b

scene = SceneClass(**scene_kwargs)

The tests are still failing after the update; they aren't reading the config variables correctly. One error I got was NameError: name \'FRAME_WIDTH\' is not defined.

But you can't get that far without patching the testing code because it's currently reading the wrong path and won't tell you what path it's failing to read since the error is caught and another one is thrown in its place:

manim/tests/testing_utils.py

Lines 57 to 63 in b660303

	with pytest.raises(FileNotFoundError) as e_info:
	data_loaded = np.load(os.path.join(
	self.path_tests_data, "{}.npy".format(str(self.scene))))
	raise FileNotFoundError('test_data not found !')
	assert (str(e_info.value) ==
	'test_data not found !'), f"{str(self.scene).replace('Test', '')} does not seem have a pre-rendered frame for testing, or it has not been found."
	return data_loaded

The scene data shouldn't be loaded this way. Instead we should just attempt to load the file and let the exception be thrown if it fails.

[eulertour]

Actually even the tests that don't run into errors loading config variables are failing from another error: fixture 'get_config_test' not found. It seems like the tests are causing more problems than they're solving here, so if the other problems are fixed I'd be fine ignoring them.

[huguesdevimeux]

Actually even the tests that don't run into errors loading config variables are failing from another error: fixture 'get_config_test' not found. It seems like the tests are causing more problems than they're solving here, so if the other problems are fixed I'd be fine ignoring them.

This is a very easy fix, get_config_test is a pytest fixture that is now deprecated (because of this PR). To me, the tests are working fine, they are just not updated yet. We can ignore them if you want, but then I don’t get the point of having tests if we ignore them every time they fail

[huguesdevimeux]

The scene data shouldn't be loaded this way. Instead we should just attempt to load the file and let the exception be thrown if it fails.

I designed that to be used by pytest, and it appears that it is the correct way to do it when one wants to catch a custom exception (see https://docs.pytest.org/en/stable/assert.html).
If we do what you're suggesting, we won't be able to show that "x has not pre-rendered data blahblah".

[leotrs]

I see three different comments here.

1. The change from SceneClass(**scene_kwargs) to SceneClass() is intentional and necessary. This is because the scene_kwargs were passing around values that can now be read through the global config. Also, I don't see why this would cause the rendering to be slower.

2. The constant FRAME_WIDTH has been deprecated and is now config['frame_width']. I just caught and changed two instances of the former that I missed.

3. The fixture get_config_test has been deprecated, as @huguesdevimeux pointed out. Similarly to **scene_kwargs, this was being used to get some test-specific config values to the scenes to be tested. These are now specified in tests/manim.cfg. I just caught and changed some instances that I missed.

Also, importantly, I can't believe I failed to say this: the command pytest is now meant to be ran from within the tests/ directory, not from the root directory. This is because of how the config files work in a cascading way, so pytest must be ran from within the appropriate directory to read the tests/manim.cfg file. @eulertour perhaps this interacts with point 1.?

[eulertour]

@huguesdevimeux

To me, the tests are working fine, they are just not updated yet. We can ignore them if you want, but then I don’t get the point of having tests if we ignore them every time they fail

Since this PR has already been blocking development for weeks, we've been developing without any tests up to this point for better or for worse, and adding the tests are causing more problems to solve, I'd be fine with it in this instance. I do think all of the test-related issues should be fixed eventually.

I designed that to be used by pytest, and it appears that it is the correct way to do it when one wants to catch a custom exception (see https://docs.pytest.org/en/stable/assert.html).

The page you linked is describing how to write assertions about raised exceptions, but in load_data() we need only raise an exception if the can't be loaded. There's no need to use a custom exception and in this case it works against us since we can't see the file that's failing to load.

If we do what you're suggesting, we won't be able to show that "x has not pre-rendered data blahblah".

This is true, but we will see No such file or directory: '<attempted file>' which is more informative for debugging.

@leotrs

The change from SceneClass(**scene_kwargs) to SceneClass() is intentional and necessary. This is because the scene_kwargs were passing around values that can now be read through the global config.

Gotcha, that's fine.

Also, I don't see why this would cause the rendering to be slower.

It's basically this snippet

manim/manim/scene/scene.py

Lines 682 to 686 in 4b6492b

	if self.skip_animations and not override_skip_animations:
	times = [run_time]
	else:
	step = 1 / self.camera.frame_rate
	times = np.arange(0, run_time, step)

scene.py checks that instance variable in order to determine whether or not to skip to the end of an animation and since it wasn't being set on the instance it was no longer skipping when -s was passed.

Points 2 and 3 are fine though I don't think the config files are causing the problem with the tests, at least not from what I've seen so far.

[huguesdevimeux]

Since this PR has already been blocking development for weeks, we've been developing without any tests up to this point for better or for worse, and adding the tests are causing more problems to solve, I'd be fine with it in this instance. I do think all of the test-related issues should be fixed eventually.

Alright, as you wish.

This is true, but we will see No such file or directory: '<attempted file>' which is more informative for debugging.

I see, you are right. thank you very much for this comment.
maybe @leotrs can quickly fix this in this PR ?

[eulertour]

I actually wanted to add something related to the cascading config files: I don't think config files from the directory where we start manim should be considered. Instead we should only use the global config, one in the home directory, and one in the directory where the script is located.

The reasoning is that it doesn't make much sense for a Scene to render differently depending on the place where it's run.

[leotrs]

Fully agreed with you there RE the cascading files.