Lazyflow logging and fix default operator naming #2850
Conversation
btbest
force-pushed
the
lazyflow-logging
branch
from
d401087
to
bfc52e9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2850 +/- ##
==========================================
- Coverage 55.75% 55.72% -0.04%
==========================================
Files 533 533
Lines 62207 62243 +36
Branches 8511 8526 +15
==========================================
Hits 34683 34683
- Misses 25769 25789 +20
- Partials 1755 1771 +16 ☔ View full report in Codecov by Sentry. |
lazyflow/slot.py
Outdated
| @@ -826,6 +829,8 @@ def get(self, roi): | |||
| ), "This inputSlot has no value and no upstream_slot. You can't ask for its data yet!" | |||
| # normal (outputslot) case | |||
| # --> construct heavy request object.. | |||
| if self.logger: | |||
| self.logger.debug(f"Getting data for roi={roi}") | |||
There was a problem hiding this comment.
| self.logger.debug(f"Getting data for roi={roi}") | |
| self.logger.debug(f"Getting data for {roi=}") |
would be more concise and in line with other f-strings you introduced in this changeset.
| @@ -108,6 +105,7 @@ def __init__( | |||
| allow_mask=False, | |||
| subindex=(), | |||
| top_level_slot=None, | |||
| write_logs=False, | |||
There was a problem hiding this comment.
This is kinda neat, to enable per slot debug messages which would otherwise be overkill via a config. Seeing this I wonder if this would not also be nice to have selective per instance logging for only some operators.
There was a problem hiding this comment.
I actually thought it was a bit ugly, adding a parameter and what feels like too much logic purely for debug logging. Plus, it's effectively logging config outside of the logging_config. But in a pixel classification example, this prevents bloating the already existing 469 loggers up to 901 loggers, so I figured it might be worth it...
This avoids instantiating dozens of loggers outside of debugging, but allows fine-grained control during debugging. * Move old debugs to the new per-slot loggers * Move old errors to a new module-level logger that always exists * Add more debugs for better tracing what slots are doing
This should give flexible control. Logs from operator.py for a specific op (e.g. `lazyflow.op_debug.OpPixelClassification`) can be separately controlled from the individual module's logging for the same op (e.g. `ilastik.applets.pixelClassification.opPixelClassification`).
Some slots put unicode characters in the slot's meta dict. E.g. `OpPixelFeaturesPresmoothed.Output.meta.channel_names` will contain the sigma character. On Windows, the log file is encoded with cp1252 by default, which cannot handle such characters and raises UnicodeEncodeError.
Logging f"{slot}" is therefore not possible by default, as the slot's repr will contain the full meta dict along with the unicode chars.
So that default name is available during subclass.__init__ and slots instantiated can use it in their logger name.
This was always defaulting to "Wrapped Operator (base class)"
Now that debug logging actually works, this introduces a bunch of new debug statements that allow tracing what lazyflow is doing. Also:
_after_init, which is too late. We want the default name to already be set at the start of__init__so that slots instantiated during__init__can put it in their logger name.At this point, the changes I make when I need to debug lazyflow are:
--debugwrite_logs=TrueinSlot.__init__instead of False to set up slot-wise loggingINFOtoDEBUGin logging_config.json for lazyflow.signals, lazyflow.graph and lazyflow.op_debugmodulefornamein theverboseformat indefault_config.py-- This is also the format for the log file in prod, otherwise I would commit this change.