Implement Logging of Terminal Output to a .log file.

[Aathish04]

Essentially what the title says.

You will be able to use the --log_to_file flag to log rich's terminal output to the default log file location, and use --log_dir <directory> to pass a custom directory for logs via the command line.

Default values for both log_to_file and log_dir have been made in defaults.cfg, and these are used as the defaults.

log_to_file defaults to False, and defaults.cfgdefaults to ./logs/

This PR should close #138 .

[eulertour]

Is there any benefit to doing this over disabling progress bars and piping to a file?

[Aathish04]

Piping to a file means the end user would have to do something like
manim file.py > output.txt every time they want to save a log, which might get tedious.

My approach lets you configure the default action with the .cfg.

Also, say you want to monitor your animation's rendering process (how long it takes for a particular animation etc), while also saving a log. If the progress bars were disabled, it would be kind of like removing a feature when it doesn't need to be removed.

[naveen521kk]

Also, this can be done after #151 as it removes logging.py.

[leotrs]

I agree setting a default is beneficial. However, I think this should wait until #151 and/or the new cfg-writer sub-command are implemented.

[Aathish04]

Ok, so I've re-implemented the logging stuff after #151 changed some things, and I've added a test to ensure that the log file is created at the right places if the right command line arguments are passed.

Review at will, guys!

[huguesdevimeux]

Issue : I have some issues with the test you wrote.
First of all, I think it would be better if you test if the content of the .log is the one expected. If not, throw an error. (In addition of testing if .log exists).
You should create a new file for tests concerning logs. (e.g test_log.py) They should not live in test_cli as they are not directly related to cli stuff.
Now that PR of colored log is merged, why not creating a test for it ? (Maybe it’s not possible, I don’t know if colors are reflected in .log).

[Aathish04]

@huguesdevimeux

Issue : I have some issues with the test you wrote.
First of all, I think it would be better if you test if the content of the .log is the one expected. If not, throw an error. (In addition of testing if .log exists).

I'll be sure to implement this in the next commit. It's a much more thorough test, so thanks!

You should create a new file for tests concerning logs. (e.g test_log.py) They should not live in test_cli as they are not directly related to cli stuff.

I see. I put it in test_cli since it was testing whether the --log_to_file and --log_dir cli arguments worked.
For future reference, where do you think the line should be drawn for stuff that depends on command line arguments?

Now that PR of colored log is merged, why not creating a test for it ? (Maybe it’s not possible, I don’t know if colors are reflected in .log).

I'm fairly sure rich doesn't transfer over colours to log files since they are just text files. I'll see if there's some way to check the colours of the output.

[Aathish04]

@huguesdevimeux I'm afraid checking testing the logging functionality by ensuring the contents of the log match an expected log behaves too erratically to be considered a "test".

Firstly, the logs themselves differ between each operating system as the paths will be different. This can be fixed with RegEx and ignoring paths. However, since Rich also records the timestamps of each command, and these will be different every time a test happens, there is no way to have a single log file with which to compare and verify.

Further, the I think rich formats differently even depending on the version of python it's running on. This causes the indentation to not line up even for the same OS and is just generally unstable.

You can see my hideous regex that tries to account for all of this here: https://github.com/Aathish04/manim/blob/366d202cecc6f7847c872caea2c548aebd60b804/tests/test_logging/test_logging.py#L27-L31

And you can see the erratic results of the tests here : https://github.com/Aathish04/manim/actions/runs/176767019

Note that it works for Linux and Macos with python3.7 and 3.8, but fails on Macos Py3.6 and windows.

[Aathish04]

@huguesdevimeux I've changed how the logs are tested.

Instead of doing a character per character check, now, the test checks if the keywords in the log are the same as those expected.
I think this should work much better. Your thoughts?

[huguesdevimeux]

Instead of doing a character per character check, now, the test checks if the keywords in the log are the same as those expected.
I think this should work much better. Your thoughts?

Why not, but this is not optimal. But I'm still convinced that .log should be identical regardless of the OS.
But this is my opinion; maybe others would have a different one.

[Aathish04]

@huguesdevimeux

Why not, but this is not optimal. But I'm still convinced that .log should be identical regardless of the OS.

If I could find a way to force rich to output at a particular size, this would be much much easier. Having read through the docs one more I couldn't find any way to override the autosizing that rich does.

@leotrs suggested stripping away everything that could possibly be different, and just comparing the things that absolutely should be the same.
This actually isn't too far from what I'm doing currently ; instead of comparing strings, I'm comparing lists of the things that shouldn't change instead.

[huguesdevimeux]

I think this PR is ready to be merged, is'nt it ?

Just a last request (rather a if-minor suggestion) : Why not implement a function that would format the logs to a consistent format regardless of the OS? It could be then used in the tests. I don't know if it is possible and if it is a good idea, tell me.

[Aathish04]

Just a last request (rather a if-minor suggestion) : Why not implement a function that would format the logs to a consistent format regardless of the OS? It could be then used in the tests. I don't know if it is possible and if it is a good idea, tell me.

I don't think it is possible. If it is, then someone with more experience formatting things with rich will probably know. When I read through rich's docs, I found no utility to resize the output that rich gives.

[huguesdevimeux]

I see. I just look at the documentation, and did you try to specify height and width in the console instantiation? According to the documentation, it will prevent the auto-detection of the console size.

https://rich.readthedocs.io/en/latest/reference/console.html?highlight=size#

[Aathish04]

I see. I just look at the documentation, and did you try to specify height and width in the console instantiation? According to the documentation, it will prevent the auto-detection of the console size. https://rich.readthedocs.io/en/latest/reference/console.html?highlight=size#

To use something like this, logger.py itself will have to be modified for that test alone.

Is there any way for pytest to modify a file in this way before it does anything else and then change it back?

[huguesdevimeux]

I thought to add an option in manim.cfg in logger section to specify a console size. We could then choose a custom and constant one for the tests.

Sorry for being so insisting on this point but I'm truly convinced that it will be necessary for the future tests.

[leotrs]

Issue: this looks like it has two levels of indentation when it only needs one.

[leotrs]

Thanks! There is one open comment on my review, and the tests are failing now :(

[Aathish04]

Thanks! There is one open comment on my review.
I think running black solved the formatting.

and the tests are failing now :(

Welp, there's only one place where the error can come from. I'll have to make my regex better I think, or something of the sort.

[huguesdevimeux]

SGTM.

Looking forward the next PR that will improve the personalization of the logs :D (no pressure at all).

[huguesdevimeux]

Nitpick : Maybe document this saying that this is provisional.