Enhance logging messages with rich #869
Conversation
facebook-github-bot
added
the
CLA Signed
Etesam913
changed the title
There was a problem hiding this comment.
The duplicate logging is a symptom of hydra's default logging. This should be configurable.
With a bit of digging I'd suggest
- Editing
hydra_config.pyto include aLOGGING_OVERRIDE = {'override hydra/job_logging': 'mephisto_default'}override constant. - Creating a file called
mephisto_default.yamlin the specific folderhydra_configs/hydra/job_logging. - Include the
LOGGING_OVERRIDEas the last value in a defaults list provided both to the standardTaskConfigand the version produced bybuild_default_task_config.
Now as far as what to do when you've gotten this far, I'm not 100% sure. Either the choice is going to be to make the hydra_log_file.yaml use the rich logger instead of the simple one (more likely), or to make it so that the hydra_log_file resembles the disabled setup to block hydra's logs to console if that doesn't work.
mephisto/utils/metrics.py
Outdated
| @@ -14,7 +14,7 @@ | |||
| import subprocess | |||
| import sys | |||
| import time | |||
|
|
|||
| from mephisto.utils.rich import console | |||
|
Got it working 👍 I had to make sure that the hydra job_logging did not write results to console, but did write to file. New Videoupdated-hydra-logging.mov |
|
This is in It seems a little extra. |
My concern here is if someone has logging entirely disabled, and then doesn't get to see this message |
|
Makes sense |
Codecov Report
@@ Coverage Diff @@
## main #869 +/- ##
==========================================
+ Coverage 64.43% 64.58% +0.14%
==========================================
Files 107 108 +1
Lines 9291 9304 +13
==========================================
+ Hits 5987 6009 +22
+ Misses 3304 3295 -9
Continue to review full report at Codecov.
|
Etesam913
changed the title
mephisto/operations/hydra_config.py
Outdated
| @@ -46,6 +46,7 @@ class TaskConfig: | |||
| mephisto: MephistoConfig = MephistoConfig() | |||
| task_dir: str = get_run_file_dir() | |||
| num_tasks: int = 5 | |||
| defaults = ["_self_", LOGGING_OVERRIDE] | |||
There was a problem hiding this comment.
You'll need to use:
defaults: List[Any] = field(default_factory=lambda: ["_self_", LOGGING_OVERRIDE])here or Hydra will throw for base TaskConfigs.
| def assert_server_subbed_in_time(self, server, timeout: int = 5) -> None: | ||
| start_time = time.time() | ||
| while (len(server.subs) == 0) and time.time() - start_time < timeout: | ||
| time.sleep(0.3) | ||
| self.assertEqual( | ||
| len(self.architect.server.subs), | ||
| 1, | ||
| "MockServer doesn't see registered channel", | ||
| ) |
|
This is something I've been wanting for a long time. Thanks for making it happen ❤️ |
|
Side thought - The duplication of logs here could be an opportunity for clean-up?
|
Etesam asked about this above. The difficulty here I suppose is that I wanted the link to always appear, even if logging is disabled. A solution would be one that forces the print only if logging isn't active, but I don't know if there's any really elegant way to do that. |
Summary
Many of the scripts are beautified using rich, why not add this to the logs.
Added a
RichHandlerobject to the logging config to allow for clearer logging messages.Updated Video:
#869 (comment)
Video
enhanced-logging.mov
Duplicated Log Messages Issue (RESOLVED) #869 (comment)
One thing that I want to fix is the duplicated log messages. You can see that each colorful log message has a uncolored version.
I believe this is because the main logger is also logging the messages, which leads to duplicated logging. One way to fix this is to set
This does work, however, it means that the messages won't get saved in the
outputs/<date>/scripts.logfile. This is a pretty big issue, so I didn't apply it. Removing the duplicated messages does make the logs much easier to read though.Any ideas on how to resolve this?
Resolves #388
Resolves #386