Verbose #218

azarzadavila · 2020-07-25T15:37:40Z

Proposal to solve #107 .

This PR introduces two CLI args :

verbose : string which controls the global verbosity
ffmpeg : int which controls the log level of ffmpeg

I think they are mainly four sources of output when executing manim :

ffmpeg
logs from logger
status bar handled by tqdm
the video viewer used when previewing

I suggest to use the levels of logger as the main parameter, which can be in : DEBUG, INFO, WARNING, ERROR, CRITICAL.
The offered solution is by default to pass the global config of verbose to the logger and then to map this config for the other sources of outputs.
Namely,

DEBUG -> ffmpeg set to loglevel 16 (error level) / the status bar is printed / the output of the video viewer is captured
INFO -> ffmpeg set to loglevel 16 (error level) / the status bar is printed / the output of the video viewer is not captured
WARNING -> ffmpeg set to loglevel 16 (error level) / the status bar is not printed / the output of the video viewer is not captured
ERROR -> ffmpeg set to loglevel 16 (error level) / the status bar is not printed / the output of the video viewer is not captured
CRITICAL -> ffmpeg set to loglevel 8 (fatal level) / the status bar is not printed / the output of the video viewer is not captured

However as an user may be interested over more control for ffmpeg, one can also pass the arg --ffmpeg to override the mapping.
I also kept the --quiet option equivalent to --verbose CRITICAL.
The three discussed options can be passed on command arguments or on default.cfg but the presence of ffmpeg and quiet in the configuration file is not mandatory.

huguesdevimeux

First of all, thank you very much for this contribution.
Just two things :

I tested it and passed --ffpmeg 0,--ffpmeg -8,--ffpmeg 16 as flag (since these values are choices on the flag --ffmpeg you added), and it didn't change anything. As I'm not familiar with that at all, it would be nice if you could enlight me on the reasons.
Suggestion : Why not adding , in addition to --verbose flag, a flag that would disable progressbar ? Here, progressbar is disabled only when DEBUG or INFO is not enabled, which is cool but it woulc be better if we could have debug logs without progress bar if wanted. I would like to have other's opinion on this point.

manim/utils/config_utils.py

manim/constants.py

manim/utils/config_utils.py

huguesdevimeux · 2020-07-26T13:07:11Z

REMINDER : Please do not merge it until @leotrs approved, as it touches config and I want to be sure that it this done the correct way.

leotrs · 2020-07-26T15:51:53Z

manim/constants.py

+VERBOSE_CHOICES = ["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]
+VERBOSE_FFMPEG_MAP = {
+    "DEBUG": "error",
+    "INFO": "error",
+    "WARNING": "error",
+    "ERROR": "error",
+    "CRITICAL": "fatal",
+}


Suggestion:

VERBOSE_FFMPEG_MAP = { "DEBUG": "error", "INFO": "error", "WARNING": "error", "ERROR": "error", "CRITICAL": "fatal", } VERBOSE_CHOICES = VERBOSE_FFMPEG_MAP.keys()

leotrs · 2020-07-26T15:52:43Z

manim/default.cfg

 # -q, --quiet
 quiet = False


We should delete this, yes?

leotrs · 2020-07-26T15:53:49Z

manim/scene/scene.py

+        disable = not file_writer_config["progress_bar"]
        time_progression = ProgressDisplay(
            times, total=n_iterations,
            leave=file_writer_config['leave_progress_bars'],
-            ascii=True if platform.system() == 'Windows' else None
+            ascii=True if platform.system() == 'Windows' else None,
+            disable=disable,


Suggestion: just do time_progression = ProgressDisplay(..., disable=not file_writer_config["progress_bar"], ...).

leotrs · 2020-07-26T15:57:32Z

manim/utils/config_utils.py

+    quiet = getattr(args, "quiet")
+    if not quiet and default.getboolean("quiet", None) is not None:
+        quiet = default.getboolean("quiet")


It's a bit confusing to me to have both verbose and quiet flags. Is there any way to keep only one?

Yes absolutely quiet is just a shortcut for verbose CRITICAL. I thought it could be easier for a user that wants almost no output to remember quiet than verbose CRITICAL. However it does add an additional option so if it is confusing I'll remove it.

leotrs · 2020-07-26T15:58:22Z

manim/utils/config_utils.py

@@ -122,6 +121,27 @@ def _parse_file_writer_config(config_parser, args):
        [fw_config["save_last_frame"], fw_config["from_animation_number"]]
    )

+    # Read in the log level


Suggestion: "parse the verbose and quiet flags to read in the log level".

All other comments indicate which CLI flags they are dealing with, so this one should too.

Okey, I'll keep it in mind when modifying.

leotrs · 2020-07-26T16:06:30Z

manim/utils/config_utils.py

+    verbose = getattr(args, "verbose")
+    verbose = "CRITICAL" if verbose is None and quiet else verbose
+    verbose = default["verbose"] if verbose is None else verbose


I'm not sure I can follow this logic easily. Also, it seems that one of the conditionals here is catching the case when args doesn't have a verbose attribute, which presumably happens when the user doesn't pass the verbose flag. This is not how we want to handle things.

The config system is meant to be used as follows: if the user passes a value through CLI flag, we use that value. If they don't, we use whatever value is in the library-wide config file default.cfg. So the logic should be something like this:

verbose = args.verbose if args.verbose is not None else default["verbose"]

which takes just one line. Does this make sense? Does this yield the same logic as what you wrote before?

It does yield the same logic except if verbose is not passed in the CLI flags but if quiet is (or if quiet is in the default.cfg). If the flag quiet flag is removed I'll write it as you put it.

leotrs · 2020-07-26T16:07:08Z

manim/utils/config_utils.py

+        "-v", "--verbose",
+        type=str,
+        help="Verbosity level. Also changes the ffmpeg log level unless the latter is specified",
+        metavar="loglevel",


Question: what is a metavar? What is the difference from dest?

I think metavar is the display name when asking details through --help. On the other hand dest is the name of the attribute that will be generated.
This means that in the help message it will print --verbose loglevel but the attribute will be accessed through args.verbose.

I would strongly prefer if the print message and the attribute be called the same.

Okey, I removed the metavar attribute to keep the default behavior which is to show in the help message all possible values.

leotrs · 2020-07-26T16:07:44Z

manim/utils/config_utils.py

+        type=str,
+        help="Display the progress bar",
+        metavar="on/off",
+        choices=["on", "off"],


Suggestion: change to Boolean.

leotrs · 2020-07-26T16:09:10Z

Something I don't like about this PR is that it's adding three new CLI flags. I think some of them can be just config options. For example, I don't think we need a flag to switch between showing or not showing the progress bar. What do you all think?

huguesdevimeux · 2020-07-26T16:13:33Z

Something I don't like about this PR is that it's adding three new CLI flags. I think some of them can be just config options. For example, I don't think we need a flag to switch between showing or not showing the progress bar. What do you all think?

I strongly disagree on the flag for the progress bar, it would be a useful flag. Nevertheless, I think that ffmpeg_vebose shouldn't be a flag, maybe.

azarzadavila · 2020-07-29T10:03:34Z

I'll remove then the flag quiet and keep ffmpeg_loglevel only in config file and not in the CLI flags.
Like that the four flags of this PR become only verbose and progress_bar which can also be set in default.cfg.

azarzadavila · 2020-07-29T11:11:11Z

I also removed the fact that changing the verbose flag can change the progress_bar as to make it simpler.
I put the loglevel for ffmpeg into a new section [ffmpeg] in the config file. I am thinking it may be useful if in the future it is decided to give the user even more control to the ffmpeg flags.

leotrs · 2020-08-02T15:42:47Z

@azarzadavila sounds good to me! Can you please resolve the conflicts before we can merge this?

leotrs · 2020-08-02T15:43:03Z

Also, when this is merged please remember to close #107

leotrs · 2020-08-02T15:43:49Z

manim/utils/config_utils.py

+        "-v", "--verbose",
+        type=str,
+        help="Verbosity level. Also changes the ffmpeg log level unless the latter is specified",
+        metavar="loglevel",


I would strongly prefer if the print message and the attribute be called the same.

manim/utils/config_utils.py

azarzadavila · 2020-08-04T09:16:07Z

I also see there is now a new attribute log_level in the config [logger] in default.cfg from a commit a few hours earlier.
I am guessing it would have the same function as the attribute verbose but I did not change it not knowing for sure it had the same purpose.

leotrs · 2020-08-04T13:25:38Z

@Aathish04 and @naveen521kk can you please comment on how this relates to #180? Thanks.

naveen521kk · 2020-08-04T13:49:18Z

No, those things in default.cfg for colour and not related to verbose output. @azarzadavila

Aathish04 · 2020-08-04T13:49:39Z

I also see there is now a new attribute log_level in the config [logger] in default.cfg from a commit a few hours earlier.
@Aathish04 and @naveen521kk can you please comment on how this relates to #180? Thanks.

log_level was introduced in commit d5162f5 , and not by #180. I have tried changing its values multiple times, and cannot find out what it actually modifies.

I think @naveen521kk is in a better position to answer what changing log_level actually modifies, since it was introduced with his PR and he has more experience with rich than me.

leotrs · 2020-08-04T13:51:18Z

No, those things in default.cfg for colour and not related to verbose output. @azarzadavila

Sounds good! Now the problem will be that we will now have two flags log_level and verbose, which sound redundant, and it's not obvious which one does what.

azarzadavila · 2020-08-05T16:04:51Z

I agree the names are confusing between those two flags. I see log_level is currently (in the branch master) not used anywhere and is only present in the config files. If log_level is related to the color of the output than it could perhaps be indicated in its name and thus reducing the ambiguity between the two flags ?

leotrs · 2020-08-05T17:33:30Z

@naveen521kk could you tell us exactly what log_level does? I see it exists as a possible config option in the config files, but there is no CLI flag for it.

azarzadavila added 8 commits July 24, 2020 19:31

Recognize verbose in config

87fd921

Pass verbose to ffmpeg

07512aa

Separate verbosity level of project with ffmpeg loglevel

48827d5

!fiuxp remove SILENT in loglevel

707395b

Show ouput of open_file_if_needed only on DEBUG

4f39036

Quiet equivalent to CRITICAL level

3635ec7

Disable status bar if verbose level not DEBUG/INFO

a75611d

Remove comment set logger level

b2bdbe9

huguesdevimeux requested changes Jul 25, 2020

View reviewed changes

manim/utils/config_utils.py Outdated Show resolved Hide resolved

manim/constants.py Outdated Show resolved Hide resolved

manim/utils/config_utils.py Outdated Show resolved Hide resolved

azarzadavila added 3 commits July 25, 2020 20:00

Add option progress_bar on/off

eee9874

Change ffmpeg loglevel to a string

624b6ab

Change ffmpeg option to ffmpeg_loglevel

4a49e4a

azarzadavila requested a review from huguesdevimeux July 25, 2020 19:29

Aathish04 added the enhancement Additions and improvements in general label Jul 25, 2020

huguesdevimeux approved these changes Jul 26, 2020

View reviewed changes

Aathish04 linked an issue Jul 26, 2020 that may be closed by this pull request

--quiet not optimal #107

Closed

leotrs suggested changes Jul 26, 2020

View reviewed changes

VERBOSE_CHOICES through dict keys from FFMPEG_MAP

794c8e0

azarzadavila added 6 commits July 29, 2020 12:12

Remove quiet flag

1988237

Remove disable var for progress_bar

6f208e4

ffmpeg_loglevel only in config file

520d1c0

progress_bar flag to boolean

ce7cecb

progress_bar independant on verbose flag

6209cdf

Comments on verbose/ffmpeg loglevel/progress_bar flags

0104d6e

leotrs suggested changes Aug 2, 2020

View reviewed changes

azarzadavila added 4 commits August 3, 2020 19:01

Remove metavar to verbose

67c12fc

Merge remote-tracking branch 'fork/master' into verbose

7c1c11b

Black config_utils.py

097998a

Merge remote-tracking branch 'fork/master' into verbose

9498cfa

huguesdevimeux approved these changes Aug 9, 2020

View reviewed changes

leotrs approved these changes Aug 10, 2020

View reviewed changes

leotrs merged commit 74c158c into ManimCommunity:master Aug 10, 2020

leotrs mentioned this pull request Aug 14, 2020

Don't show the config files every time #287

Closed

Verbose #218

Verbose #218

Uh oh!

Conversation

azarzadavila commented Jul 25, 2020

Uh oh!

huguesdevimeux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huguesdevimeux commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leotrs commented Jul 26, 2020

Uh oh!

huguesdevimeux commented Jul 26, 2020

Uh oh!

azarzadavila commented Jul 29, 2020

Uh oh!

azarzadavila commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leotrs commented Aug 2, 2020

Uh oh!

leotrs commented Aug 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azarzadavila commented Aug 4, 2020

Uh oh!

leotrs commented Aug 4, 2020

Uh oh!

naveen521kk commented Aug 4, 2020

Uh oh!

Aathish04 commented Aug 4, 2020

Uh oh!

leotrs commented Aug 4, 2020

Uh oh!

azarzadavila commented Aug 5, 2020

Uh oh!

leotrs commented Aug 5, 2020

Uh oh!

Uh oh!

huguesdevimeux commented Jul 26, 2020 •

edited

Loading

azarzadavila commented Jul 29, 2020 •

edited

Loading