Fixed a File logging bug and some maintenance (ManimCommunity#2546)

* Fixed File logging bug

Previously, the file logger was set in manim/_config/utils.
That caused problems as python tried to enable file logging when other
variables like media_dir, scene_name and others which were required for
the naming of log_dir were not yet set.

Now, we enable log_to_file in scene_file_writer where all the required
directories are declared before we enable file logging.

Also removed a test that was based on set_file_logger's previous
behaviour where it added a suffix only when the class name was
specified when calling manim. Since, we now enable file logging
in scene_file_writer, we always have access to the class name
regardless.

* Added Typings to file_ops

* Fixed typings and some warnings in _config.utils

Fixed some typings to now explicitly importing the types we require.
Removed one of the double declarations of dry_run.
Removed an unused import and a block of commented code.

* Remove rendundant check

* Add suggested changes

Co-authored-by: Hugues Devimeux <[email protected]>

Author: ad-chaos

manim/_config/logger_utils.py

@@ -15,7 +15,6 @@
 import copy
 import json
 import logging
-import os
 import sys
 from typing import TYPE_CHECKING
 
@@ -26,7 +25,7 @@
 from rich.theme import Theme
 
 if TYPE_CHECKING:
-    from manim._config.utils import ManimConfig
+    from pathlib import Path
 HIGHLIGHTED_KEYWORDS = [  # these keywords are highlighted specially
     "Played",
     "animations",
@@ -142,7 +141,7 @@ def parse_theme(parser: configparser.ConfigParser) -> Theme:
     return custom_theme
 
 
-def set_file_logger(config: ManimConfig, verbosity: str) -> None:
+def set_file_logger(scene_name: str, module_name: str, log_dir: Path) -> None:
     """Add a file handler to manim logger.
 
     The path to the file is built using ``config.log_dir``.
@@ -152,28 +151,13 @@ def set_file_logger(config: ManimConfig, verbosity: str) -> None:
     config : :class:`ManimConfig`
         The global config, used to determine the log file path.
 
-    verbosity : :class:`str`
-        The verbosity level of the logger.
-
-    Notes
-    -----
-    Calling this function changes the verbosity of all handlers assigned to
-    manim logger.
-
     """
     # Note: The log file name will be
     # <name_of_animation_file>_<name_of_scene>.log, gotten from config.  So it
     # can differ from the real name of the scene.  <name_of_scene> would only
     # appear if scene name was provided when manim was called.
-    scene_name_suffix = "".join(config["scene_names"])
-    scene_file_name = os.path.basename(config["input_file"]).split(".")[0]
-    log_file_name = (
-        f"{scene_file_name}_{scene_name_suffix}.log"
-        if scene_name_suffix
-        else f"{scene_file_name}.log"
-    )
-    log_file_path = config.get_dir("log_dir") / log_file_name
-    log_file_path.parent.mkdir(parents=True, exist_ok=True)
+    log_file_name = f"{module_name}_{scene_name}.log"
+    log_file_path = log_dir / log_file_name
 
     file_handler = logging.FileHandler(log_file_path, mode="w")
     file_handler.setFormatter(JSONFormatter())
@@ -182,9 +166,6 @@ def set_file_logger(config: ManimConfig, verbosity: str) -> None:
     logger.addHandler(file_handler)
     logger.info("Log file will be saved in %(logpath)s", {"logpath": log_file_path})
 
-    config.verbosity = verbosity
-    logger.setLevel(verbosity)
-
 
 class JSONFormatter(logging.Formatter):
     """A formatter that outputs logs in a custom JSON format.

manim/_config/utils.py

@@ -19,17 +19,16 @@
 import os
 import re
 import sys
-import typing
 from collections.abc import Mapping, MutableMapping
 from pathlib import Path
+from typing import Any, Iterable, Iterator
 
 import colour
 import numpy as np
 
 from .. import constants
 from ..utils.tex import TexTemplate, TexTemplateFromFile
 from ..utils.tex_templates import TexTemplateLibrary
-from .logger_utils import set_file_logger
 
 
 def config_file_paths() -> list[Path]:
@@ -302,7 +301,7 @@ def __init__(self) -> None:
         self._d = {k: None for k in self._OPTS}
 
     # behave like a dict
-    def __iter__(self) -> typing.Iterator[str]:
+    def __iter__(self) -> Iterator[str]:
         return iter(self._d)
 
     def __len__(self) -> int:
@@ -315,10 +314,10 @@ def __contains__(self, key) -> bool:
         except AttributeError:
             return False
 
-    def __getitem__(self, key) -> typing.Any:
+    def __getitem__(self, key) -> Any:
         return getattr(self, key)
 
-    def __setitem__(self, key: str, val: typing.Any) -> None:
+    def __setitem__(self, key: str, val: Any) -> None:
         getattr(ManimConfig, key).fset(self, val)  # fset is the property's setter
 
     def update(self, obj: ManimConfig | dict) -> None:
@@ -393,7 +392,7 @@ def __copy__(self) -> ManimConfig:
         """See ManimConfig.copy()."""
         return copy.deepcopy(self)
 
-    def __deepcopy__(self, memo: dict[str, typing.Any]) -> ManimConfig:
+    def __deepcopy__(self, memo: dict[str, Any]) -> ManimConfig:
         """See ManimConfig.copy()."""
         c = ManimConfig()
         # Deepcopying the underlying dict is enough because all properties
@@ -403,14 +402,14 @@ def __deepcopy__(self, memo: dict[str, typing.Any]) -> ManimConfig:
         return c
 
     # helper type-checking methods
-    def _set_from_list(self, key: str, val: typing.Any, values: list) -> None:
+    def _set_from_list(self, key: str, val: Any, values: list) -> None:
         """Set ``key`` to ``val`` if ``val`` is contained in ``values``."""
         if val in values:
             self._d[key] = val
         else:
             raise ValueError(f"attempted to set {key} to {val}; must be in {values}")
 
-    def _set_boolean(self, key: str | int, val: typing.Any) -> None:
+    def _set_boolean(self, key: str | int, val: Any) -> None:
         """Set ``key`` to ``val`` if ``val`` is Boolean."""
         if val in [True, False]:
             self._d[key] = val
@@ -423,7 +422,7 @@ def _set_tuple(self, key: str, val: tuple) -> None:
         else:
             raise ValueError(f"{key} must be tuple")
 
-    def _set_str(self, key: str, val: typing.Any) -> None:
+    def _set_str(self, key: str, val: Any) -> None:
         """Set ``key`` to ``val`` if ``val`` is a string."""
         if isinstance(val, str):
             self._d[key] = val
@@ -830,8 +829,7 @@ def digest_file(self, filename: str) -> ManimConfig:
                 filename,
             )
 
-        if filename:
-            return self.digest_parser(make_config_parser(filename))
+        return self.digest_parser(make_config_parser(filename))
 
     # config options are properties
     preview = property(
@@ -856,19 +854,11 @@ def digest_file(self, filename: str) -> ManimConfig:
         doc="Whether to show progress bars while rendering animations.",
     )
 
-    @property
-    def log_to_file(self):
-        """Whether to save logs to a file."""
-        return self._d["log_to_file"]
-
-    @log_to_file.setter
-    def log_to_file(self, val: str) -> None:
-        self._set_boolean("log_to_file", val)
-        if val:
-            log_dir = self.get_dir("log_dir")
-            if not os.path.exists(log_dir):
-                os.makedirs(log_dir)
-            set_file_logger(self, self["verbosity"])
+    log_to_file = property(
+        lambda self: self._d["log_to_file"],
+        lambda self, val: self._set_boolean("log_to_file", val),
+        doc="Whether to save logs to a file.",
+    )
 
     notify_outdated_version = property(
         lambda self: self._d["notify_outdated_version"],
@@ -924,12 +914,6 @@ def log_to_file(self, val: str) -> None:
         doc="Set to force window when using the opengl renderer",
     )
 
-    dry_run = property(
-        lambda self: self._d["dry_run"],
-        lambda self, val: self._set_boolean("dry_run", val),
-        doc="Enable dry_run so that no output files are generated and window is disabled.",
-    )
-
     @property
     def verbosity(self):
         """Logger verbosity; "DEBUG", "INFO", "WARNING", "ERROR", or "CRITICAL" (-v)."""
@@ -1610,15 +1594,15 @@ def __init__(self, c: ManimConfig) -> None:
         self.__dict__["_c"] = c
 
     # there are required by parent class Mapping to behave like a dict
-    def __getitem__(self, key: str | int) -> typing.Any:
+    def __getitem__(self, key: str | int) -> Any:
         if key in self._OPTS:
             return self._c[key]
         elif key in self._CONSTANTS:
             return self._CONSTANTS[key]
         else:
             raise KeyError(key)
 
-    def __iter__(self) -> typing.Iterable:
+    def __iter__(self) -> Iterable:
         return iter(list(self._OPTS) + list(self._CONSTANTS))
 
     def __len__(self) -> int:

manim/scene/scene_file_writer.py

@@ -19,6 +19,7 @@
 from manim import __version__
 
 from .. import config, logger
+from .._config.logger_utils import set_file_logger
 from ..constants import FFMPEG_BIN, GIF_FILE_EXTENSION
 from ..utils.file_ops import (
     add_extension_if_not_present,
@@ -158,6 +159,12 @@ def init_output_directories(self, scene_name):
                 ),
             )
 
+            if config["log_to_file"]:
+                log_dir = guarantee_existence(config.get_dir("log_dir"))
+                set_file_logger(
+                    scene_name=scene_name, module_name=module_name, log_dir=log_dir
+                )
+
     def finish_last_section(self) -> None:
         """Delete current section if it is empty."""
         if len(self.sections) and self.sections[-1].is_empty():

manim/utils/file_ops.py

@@ -24,6 +24,10 @@
 import time
 from pathlib import Path
 from shutil import copyfile
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from ..scene.scene_file_writer import SceneFileWriter
 
 from manim import __version__, config, logger
 
@@ -117,33 +121,35 @@ def write_to_movie() -> bool:
     )
 
 
-def add_extension_if_not_present(file_name, extension):
+def add_extension_if_not_present(file_name: Path, extension: str) -> Path:
     if file_name.suffix != extension:
         return file_name.with_suffix(extension)
     else:
         return file_name
 
 
-def add_version_before_extension(file_name):
+def add_version_before_extension(file_name: Path) -> Path:
     file_name = Path(file_name)
     path, name, suffix = file_name.parent, file_name.stem, file_name.suffix
     return Path(path, f"{name}_ManimCE_v{__version__}{suffix}")
 
 
-def guarantee_existence(path):
+def guarantee_existence(path: Path) -> Path:
     if not os.path.exists(path):
         os.makedirs(path)
-    return os.path.abspath(path)
+    return Path(os.path.abspath(path))
 
 
-def guarantee_empty_existence(path):
+def guarantee_empty_existence(path: Path) -> Path:
     if os.path.exists(path):
         shutil.rmtree(path)
     os.makedirs(path)
-    return os.path.abspath(path)
+    return Path(os.path.abspath(path))
 
 
-def seek_full_path_from_defaults(file_name, default_dir, extensions):
+def seek_full_path_from_defaults(
+    file_name: Path, default_dir: str, extensions: str
+) -> Path:
     possible_paths = [file_name]
     possible_paths += [
         Path(default_dir) / f"{file_name}{extension}" for extension in ["", *extensions]
@@ -155,7 +161,7 @@ def seek_full_path_from_defaults(file_name, default_dir, extensions):
     raise OSError(error)
 
 
-def modify_atime(file_path):
+def modify_atime(file_path) -> None:
     """Will manually change the accessed time (called `atime`) of the file, as on a lot of OS the accessed time refresh is disabled by default.
 
     Parameters
@@ -188,7 +194,7 @@ def open_file(file_path, in_browser=False):
         sp.Popen(commands)
 
 
-def open_media_file(file_writer):
+def open_media_file(file_writer: SceneFileWriter) -> None:
     file_paths = []
 
     if config["save_last_frame"]:
@@ -207,7 +213,7 @@ def open_media_file(file_writer):
             logger.info(f"Previewed File at: '{file_path}'")
 
 
-def get_template_names():
+def get_template_names() -> list[str]:
     """Returns template names from the templates directory.
 
     Returns
@@ -218,7 +224,7 @@ def get_template_names():
     return [template_name.stem for template_name in template_path.glob("*.mtp")]
 
 
-def get_template_path():
+def get_template_path() -> Path:
     """Returns the Path of templates directory.
 
     Returns
@@ -242,7 +248,9 @@ def add_import_statement(file):
         f.write(import_line.rstrip("\r\n") + "\n" + content)
 
 
-def copy_template_files(project_dir=Path("."), template_name="Default"):
+def copy_template_files(
+    project_dir: Path = Path("."), template_name: str = "Default"
+) -> None:
     """Copies template files from templates dir to project_dir.
 
     Parameters

tests/test_config.py

@@ -98,6 +98,7 @@ def test_custom_dirs(tmp_path):
         {
             "media_dir": tmp_path,
             "save_sections": True,
+            "log_to_file": True,
             "frame_rate": 15,
             "pixel_height": 854,
             "pixel_width": 480,
@@ -108,6 +109,7 @@ def test_custom_dirs(tmp_path):
             "images_dir": "{media_dir}/test_images",
             "text_dir": "{media_dir}/test_text",
             "tex_dir": "{media_dir}/test_tex",
+            "log_dir": "{media_dir}/test_log",
         }
     ):
         scene = MyScene()
@@ -131,7 +133,7 @@ def test_custom_dirs(tmp_path):
 
         assert_dir_filled(os.path.join(tmp_path, "test_text"))
         assert_dir_filled(os.path.join(tmp_path, "test_tex"))
-        # TODO: testing the log dir would be nice but it doesn't get generated for some reason and test crashes when setting "log_to_file" to True
+        assert_dir_filled(os.path.join(tmp_path, "test_log"))
 
 
 def test_frame_size(tmp_path):

tests/test_logging/test_logging.py

@@ -35,32 +35,6 @@ def test_logging_to_file(tmp_path, python_version):
     assert exitcode == 0, err
 
 
-@logs_comparison(
-    "BasicSceneLoggingTest.txt",
-    os.path.join("logs", "basic_scenes_square_to_circle.log"),
-)
-def test_logging_when_scene_is_not_specified(tmp_path, python_version):
-    path_basic_scene = os.path.join(
-        "tests",
-        "test_logging",
-        "basic_scenes_square_to_circle.py",
-    )
-    command = [
-        python_version,
-        "-m",
-        "manim",
-        "-ql",
-        "-v",
-        "DEBUG",
-        "--log_to_file",
-        "--media_dir",
-        str(tmp_path),
-        path_basic_scene,
-    ]
-    _, err, exitcode = capture(command)
-    assert exitcode == 0, err
-
-
 def test_error_logging(tmp_path, python_version):
     path_error_scene = Path("tests/test_logging/basic_scenes_error.py")