Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazyflow logging and fix default operator naming #2850

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions ilastik/ilastik_logging/default_config.py
Expand Up @@ -127,6 +127,7 @@ def get_default_config(
"maxBytes": 50e6, # 20 MB
"backupCount": 5,
"formatter": "verbose",
"encoding": "utf-8",
},
},
"root": {"handlers": root_handlers, "level": root_log_level},
Expand Down
4 changes: 2 additions & 2 deletions ilastik/ilastik_logging/logging_config.json
Expand Up @@ -27,13 +27,13 @@
"ilastik.utility": { "level":"INFO" },
"ilastik.utility.exportingOperator": { "level":"INFO" },
"ilastik.utility.exportFile": { "level":"INFO" },
"lazyflow.signals": { "level":"INFO" },
"lazyflow.request": { "level":"INFO" },
"lazyflow.request.RequestLock": { "level":"INFO" },
"lazyflow.request.SimpleRequestCondition": { "level":"INFO" },
"lazyflow.graph": { "level":"INFO" },
"lazyflow.graph.Slot": { "level":"INFO" },
"lazyflow.slot.Slot": { "level":"INFO" },
"lazyflow.classifiers": { "level":"INFO" },
"lazyflow.op_debug": { "level":"INFO" },
"lazyflow.operatorWrapper.OperatorWrapper": { "level":"INFO" },
"lazyflow.operators.ioOperators": { "level":"INFO" },
"lazyflow.operators.ioOperators.opRESTfulVolumeReader": { "level":"INFO" },
Expand Down
5 changes: 4 additions & 1 deletion lazyflow/graph.py
Expand Up @@ -63,6 +63,7 @@ def call_when_setup_finished(self, fn):
fn()
else:
# Subscribe to the next completion.
logger.debug(f"Adding to queue in setup={id(self._sig_setup_complete)}: {fn}")
self._sig_setup_complete.subscribe(fn)

class SetupDepthContext(object):
Expand All @@ -79,6 +80,7 @@ def __enter__(self):
with self._graph._lock:
if self._graph._setup_depth == 0:
self._graph._sig_setup_complete = OrderedSignal()
logger.debug(f"New setupDepthContext={id(self._graph._sig_setup_complete)}")
self._graph._setup_depth += 1

def __exit__(self, *args):
Expand All @@ -88,7 +90,8 @@ def __exit__(self, *args):
self._graph._setup_depth -= 1
if self._graph._setup_depth == 0:
sig_setup_complete = self._graph._sig_setup_complete
# Reset.
logger.debug(f"Finish setupDepthContext={id(sig_setup_complete)}")
self._graph._sig_setup_complete = None
if sig_setup_complete:
logger.debug(f"Flushing queue of setupDepthContext={id(sig_setup_complete)}")
sig_setup_complete()
17 changes: 12 additions & 5 deletions lazyflow/operator.py
Expand Up @@ -173,9 +173,6 @@

"""

loggerName = __name__ + ".Operator"
logger = logging.getLogger(loggerName)

# definition of inputs slots
inputSlots = []

Expand Down Expand Up @@ -221,6 +218,9 @@
)
graph = parent.graph

if self.name == Operator.name:
self.name = type(self).__name__ # better default if subclass does not override self.name

self._cleaningUp = False
self.graph = graph
self._children = collections.OrderedDict()
Expand Down Expand Up @@ -266,9 +266,8 @@

# continue initialization, when user overrides __init__
def _after_init(self):
# provide simple default name for lazy users
if self.name == Operator.name:
self.name = type(self).__name__
self.name = type(self).__name__ # repeat in case subclass.__init__ does not call super().__init__()

Check warning on line 270 in lazyflow/operator.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/operator.py#L270

Added line #L270 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.name = type(self).__name__ # repeat in case subclass.__init__ does not call super().__init__()

I wouldn't want to enable/start supporting this kind of pattern where operators are created without calling super().__init__. I checked and all operators we currently have in code do the right thing. The operator base init is sufficiently complex that I would expect all subclasses to call it. Also this then adds a third place where .name might be mutated (after the subclass definition and super().__init__) during initialization which is a bit much.

assert self.graph is not None, (
"Operator {}: self.graph is None, the parent ({})"
" given to the operator must have a valid .graph attribute!".format(self, self._parent)
Expand Down Expand Up @@ -301,6 +300,10 @@
islot.notifyUnready(self.handleInputBecameUnready)

self._initialized = True

self.logger = logging.getLogger(f"lazyflow.op_debug.{type(self).__name__}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor issue, but it is conceivable to have operators with the same type names in different modules, which would lead to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in a scenario where somehow having identically named operators in different modules seemed like a good idea, both of them writing to the same logger most likely isn't an issue... Would you rather use f"lazyflow.op_debug.{type(self).__module__}.{type(self).__name__}" and get e.g. 'lazyflow.op_debug.ilastik.applets.dataSelection.opDataSelection.OpMultiLaneDataSelectionGroup'?

self.logger.debug(f"Instantiated {self.name} {id(self)} with parent={self.parent.name if self.parent else ''}")

self._setupOutputs()

def _instantiate_slots(self):
Expand Down Expand Up @@ -393,6 +396,7 @@
if self._parent is not None:
del self._parent._children[self]

self.logger.debug(f"Cleaning up {self.name} {id(self)}")
# Disconnect ourselves and all children
self._disconnect()

Expand Down Expand Up @@ -489,6 +493,7 @@
self._condition.wait()

self._settingUp = True
self.logger.debug(f"Starting setupOutputs on {self.name} {id(self)}")

# Keep a copy of the old metadata for comparison.
# We only trigger downstream changes if something really changed.
Expand All @@ -498,6 +503,7 @@
self.setupOutputs()
self._setup_count += 1

self.logger.debug(f"Finished setupOutputs on {self.name} {id(self)}")
self._settingUp = False
self._condition.notifyAll()

Expand Down Expand Up @@ -588,6 +594,7 @@
# We are executing the operator. Incremement the execution
# count to protect against simultaneous setupOutputs()
# calls.
self.logger.debug(f"Executing {self.name} {id(self)} slot={slot.name} for roi={str(roi)} with {kwargs=}")
self._incrementOperatorExecutionCount()
return self.execute(slot, subindex, roi, result, **kwargs)
finally:
Expand Down
2 changes: 1 addition & 1 deletion lazyflow/operatorWrapper.py
Expand Up @@ -90,7 +90,7 @@ def __init__(

self._initialized = False

if operatorClass.name == "":
if operatorClass.name == Operator.name:
self._name = "Wrapped " + operatorClass.__name__
else:
self._name = "Wrapped " + operatorClass.name
Comment on lines +93 to 96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because again I had to think here longer than I wanted, for the second time ;). It's too many name __name__ _name variations

Suggested change
if operatorClass.name == Operator.name:
self._name = "Wrapped " + operatorClass.__name__
else:
self._name = "Wrapped " + operatorClass.name
# if operator type to be wrapped does not override name attribute, use its class name instead
if operatorClass.name == Operator.name:
self._name = "Wrapped " + operatorClass.__name__
else:
self._name = "Wrapped " + operatorClass.name

also - is this an issue anymore with your changes to operator.py where you assign a proper operator name, already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default naming of operators gives their instances the class name as their default self.name, but the class remains with cls.name == "Operator (base class). This branching runs into both cases a lot because we have many with and many without name in the class definition. Now that I think about it... we already have a custom OperatorMetaClass. This is exactly the kind of stuff metaclasses are for 😄 I will see if I can move this into the meta class, as I think that will allow us to basically remove all the other default renaming.

The wrapper has ._name because .name is a property, which it is so that the setter can track whether it's been called using ._customName, which exists so that the wrapper can take the instance.name from the wrapped operator once it creates instances of it.

The switch in wrapper.__init__ is necessary as long as operator default naming only happens to instances, but I don't see the point of renaming the wrapper when it creates instances of the wrapped op. The only benefit of this extra logic is to catch if the custom self.name is set inside MyOp.__init__. Which none of our operators do. I will remove the renaming on creating wrapped instances, together with _customName, _name and the name getter/setter.

Overall the value of all this logic is pretty limited anyway, we have maybe 5 or 6 custom names in our entire code base. Those few cases where it really seems to help could also be covered with creating an empty subclass with a unique name...

Expand Down
79 changes: 50 additions & 29 deletions lazyflow/slot.py
Expand Up @@ -30,9 +30,7 @@
###############################################################################
# Python
import logging
import collections
import itertools
import threading
from functools import partial, wraps
from contextlib import contextmanager
import warnings
Expand All @@ -51,6 +49,8 @@
from lazyflow.metaDict import MetaDict
from lazyflow.utility import slicingtools, OrderedSignal

module_logger = logging.getLogger(__name__)


def is_setup_fn(func):
"""
Expand Down Expand Up @@ -81,9 +81,6 @@
Base class for InputSlot, OutputSlot
"""

loggerName = __name__ + ".Slot"
logger = logging.getLogger(loggerName)

# Allow slots to be sorted by their order of creation for debug
# output and diagramming purposes.
_global_counter = itertools.count()
Expand All @@ -108,6 +105,7 @@
allow_mask=False,
subindex=(),
top_level_slot=None,
write_logs=False,
k-dominik marked this conversation as resolved.
Show resolved Hide resolved
):
"""Constructor of the Slot class.

Expand Down Expand Up @@ -162,6 +160,13 @@
self._optional = optional
self.operator = operator
self.allow_mask = allow_mask
self.logger = None
if write_logs:
# Using module-like dot separation allows turning on/off all slot loggers at once
logger_name = f"lazyflow.slot.NoOperator.{self.name}"

Check warning on line 166 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L166

Added line #L166 was not covered by tests
if self.operator is not None:
logger_name = f"lazyflow.slot.{self.operator.name}.{self.name}"
self.logger = logging.getLogger(logger_name)

Check warning on line 169 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L168-L169

Added lines #L168 - L169 were not covered by tests

# in the case of an InputSlot this is the slot to which it is
# connected
Expand Down Expand Up @@ -519,6 +524,8 @@
)
)

if self.logger:
self.logger.debug(f"Connecting to {upstream_slot}")

Check warning on line 528 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L528

Added line #L528 was not covered by tests
self.upstream_slot = upstream_slot
notifyReady = self.upstream_slot.meta._ready and not self.meta._ready
self.meta = self.upstream_slot.meta.copy()
Expand Down Expand Up @@ -604,11 +611,10 @@
# Well, this is bad. We caused an exception while handling an exception.
# We're more interested in the FIRST exception, so print this one out and
# continue unwinding the stack with the first one.
self.logger.error("Error: Caught a secondary exception while handling a different exception.")
module_logger.error("Caught a secondary exception while handling a different exception.")

Check warning on line 614 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L614

Added line #L614 was not covered by tests
import traceback

traceback.print_exc()
pass

@is_setup_fn
def disconnect(self):
Expand All @@ -626,6 +632,8 @@
had_upstream_slot = False
if self.upstream_slot is not None:
had_upstream_slot = True
if self.logger:
self.logger.debug(f"Disconnecting from {self.upstream_slot}")

Check warning on line 636 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L636

Added line #L636 was not covered by tests
# safe to unsubscribe, even if not subscribed
self.upstream_slot._sig_unready.unsubscribe(self._handleUpstreamUnready)
try:
Expand Down Expand Up @@ -672,8 +680,8 @@
return

self._resizing = True
if self.operator is not None:
self.logger.debug("Resizing slot {} of operator {} to size {}".format(self.name, self.operator.name, size))
if self.logger:
self.logger.debug(f"Resizing to {size=}")

Check warning on line 684 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L684

Added line #L684 was not covered by tests

# call before resize callbacks
self._sig_resize(self, oldsize, size)
Expand Down Expand Up @@ -718,19 +726,14 @@
# call after insert callbacks
self._sig_insert(self, position, finalsize)

if self.logger:
self.logger.debug(f"Inserting slot {position} to {finalsize=}")

Check warning on line 730 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L730

Added line #L730 was not covered by tests

slot = self._insertNew(position)

# New slot inherits our settings
slot.backpropagate_values = self.backpropagate_values

operator_name = "<NO OPERATOR>"
if self.operator:
operator_name = self.operator.name
self.logger.debug(
"Inserting slot {} into slot {} of operator {} to size {}".format(
position, self.name, operator_name, finalsize
)
)
if propagate:
if self.upstream_slot is not None and self.upstream_slot.level == self.level:
self.upstream_slot.insertSlot(position, finalsize)
Expand All @@ -754,12 +757,8 @@
if len(self) <= finalsize:
return None
assert position < len(self)
if self.operator is not None:
self.logger.debug(
"Removing slot {} into slot {} of operator {} to size {}".format(
position, self.name, self.operator.name, finalsize
)
)
if self.logger:
self.logger.debug(f"Removing slot {position} to {finalsize=}")

Check warning on line 761 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L761

Added line #L761 was not covered by tests

# call before-remove callbacks
self._sig_remove(self, position, finalsize)
Expand Down Expand Up @@ -793,6 +792,8 @@

"""
if self._value is not None:
if self.logger:
self.logger.debug(f"Returning value={self._value}")

Check warning on line 796 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L796

Added line #L796 was not covered by tests
# this handles the case of an inputslot
# having a ._value
# --> construct cheaper request object for this case
Expand All @@ -801,6 +802,8 @@
elif self.upstream_slot is not None:
# this handles the case of an inputslot
# --> just relay the request
if self.logger:
self.logger.debug(f"Passing request to {self.upstream_slot}.")

Check warning on line 806 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L806

Added line #L806 was not covered by tests
return self.upstream_slot.get(roi)
else:
if not self.ready():
Expand All @@ -826,6 +829,8 @@
), "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=}")

Check warning on line 833 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L833

Added line #L833 was not covered by tests
execWrapper = Slot.RequestExecutionWrapper(self, roi)
request = Request(execWrapper)

Expand Down Expand Up @@ -961,9 +966,6 @@
# Something is wrong. Are we cancelled?
Request.raise_if_cancelled()
if not self.ready():
# msg = "This slot ({}.{}) isn't ready yet, which means " \
# "you can't ask for its data. Is it connected?".format(self.operator and self.operator.name, self.name)
# self.logger.error(msg)
problem_slot = Slot._findUpstreamProblemSlot(self)
problem_str = str(problem_slot)
if isinstance(problem_slot, Slot):
Expand All @@ -978,7 +980,6 @@
"First upstream problem slot is: {}"
"".format(self.operator and self.operator.__class__, self.name, problem_str)
)
# self.logger.error(slotInfoMsg)
raise Slot.SlotNotReadyError(slotInfoMsg)
assert self.meta.shape is not None, (
"Can't ask for slices of this slot yet:"
Expand Down Expand Up @@ -1099,6 +1100,8 @@
" Call disconnect first if that's what you really wanted."
)
elif self.upstream_slot is not None:
if self.logger:
self.logger.debug("Propagating setValue upstream.")
self.upstream_slot.setValue(value, notify, check_changed)
return

Expand Down Expand Up @@ -1139,6 +1142,8 @@
changed = not same

if changed:
if self.logger:
self.logger.debug(f"Setting {value=}")

Check warning on line 1146 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1146

Added line #L1146 was not covered by tests
# call disconnect callbacks
self._sig_disconnect(self)
self._value = value
Expand All @@ -1155,10 +1160,14 @@
# a slot with a value is ready unless the value is None.
if self._value is not None:
if self.meta._ready != True:
if self.logger:
self.logger.debug("Now ready.")

Check warning on line 1164 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1164

Added line #L1164 was not covered by tests
self.meta._ready = True
self._sig_ready(self)
else:
if self.meta._ready != False:
if self.logger:
self.logger.debug("Now unready.")

Check warning on line 1170 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1170

Added line #L1170 was not covered by tests
self.meta._ready = False
self._sig_unready(self)

Expand Down Expand Up @@ -1191,11 +1200,10 @@
# Well, this is bad. We caused an exception while handling an exception.
# We're more interested in the FIRST excpetion, so print this one out and
# continue unwinding the stack with the first one.
self.logger.error("Error: Caught a secondary exception while handling a different exception.")
module_logger.error("Caught a secondary exception while handling a different exception.")

Check warning on line 1203 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1203

Added line #L1203 was not covered by tests
import traceback

traceback.print_exc()
pass

@is_setup_fn
def setValues(self, values):
Expand Down Expand Up @@ -1269,6 +1277,7 @@
return ready

def _setReady(self):
"""Called at the end of Operator._setupOutputs to set all outputs ready."""
wasReady = self.ready()

for p in self._subSlots:
Expand All @@ -1278,6 +1287,8 @@

# If we just became ready...
if not wasReady and self.meta._ready:
if self.logger:
self.logger.debug("Now ready. Notifying downstream slots.")

Check warning on line 1291 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1291

Added line #L1291 was not covered by tests
# Notify downstream_slots of changed readystatus
self._changed()
self._sig_ready(self)
Expand Down Expand Up @@ -1338,6 +1349,14 @@
def _changed(self):
old_ready = self.ready()
if self.upstream_slot is not None and self.meta != self.upstream_slot.meta:
if self.logger:
msg = (

Check warning on line 1353 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1353

Added line #L1353 was not covered by tests
f"Copying meta."
f" Ready: {self.meta._ready} -> {self.upstream_slot.meta._ready}."
f" Shape: {self.meta.shape} -> {self.upstream_slot.meta.shape}."
f" From {self.upstream_slot}. Previous: {self.meta}."
)
self.logger.debug(msg)

Check warning on line 1359 in lazyflow/slot.py

View check run for this annotation

Codecov / codecov/patch

lazyflow/slot.py#L1359

Added line #L1359 was not covered by tests
self.meta = self.upstream_slot.meta.copy()

if self._type == "output":
Expand Down Expand Up @@ -1377,6 +1396,8 @@

"""
if self.operator is not None:
if self.logger:
self.logger.debug(f"Calling setupOutputs of {id(self.operator)}.")
# check whether all slots are connected and notify operator
self.operator._setupOutputs()

Expand Down