34 add propper logging #36
Conversation
src/segmentmytiff/logging_config.py
Outdated
| import pandas as pd | ||
|
|
||
|
|
||
| def setup_logger(name: str = None, level=logging.INFO): |
There was a problem hiding this comment.
I see the default level here is is only used for stdout_sh. When looking at the API, my intuitive feeling would be this indicates the default level of the output logger root_logger. I suggest either rename this to stdout_level, or pass this variable to the default level of root_logger. (I prefer the second)
There was a problem hiding this comment.
My idea was to have logging to files for debug and info always on, and only have the level of stdout configurable. So in that sense, the second options isn't possible. I think the first option is an excellent suggestion.
|
|
||
| # Configure logger and add handlers | ||
| root_logger = logging.getLogger() | ||
| root_logger.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Following the suggestion on level, proposing this.
| root_logger.setLevel(logging.DEBUG) | |
| root_logger.setLevel(level) |
| path = Path('log') | ||
| path.mkdir(exist_ok=True, parents=True) |
There was a problem hiding this comment.
Consider expose the path of log file as an option?
There was a problem hiding this comment.
I thought about that, but I think that adds unnecessary complexity for now. If you think we need it (now or soon), especially from the plugin side, I'll go for it of course.
| logger.info(f"{task_name} finished in {duration:.4f} seconds") | ||
|
|
||
|
|
||
| def log_array(data: np.ndarray, logger, array_name:str="array") -> None: |
There was a problem hiding this comment.
In this function, despite the level of logger, all calculations be called. In case a large data, this will significantly slow down you workflow.
Maybe we should consider check the level of logger?
IMO maybe it's better to move the calculation part out of this function and pass them as a dictionary in here. I think when calling a logging function, the computation effort is usally neglected. The user should be aware what calculation is actually done.
There was a problem hiding this comment.
Good point! I was focusing on gathering all data that we'd need to debug a user's problem, I didn't consider performance enough.
| @@ -19,7 +20,7 @@ def image_path_to_mask_path(image_path: Path) -> Path: | |||
| self.masks = [str(image_path_to_mask_path(Path(p))) for p in self.images][:self.limit] | |||
| non_existing_masks = [p for p in self.masks if Path(p).exists() == False] | |||
| if non_existing_masks: | |||
| print(f"{len(non_existing_masks)} of a total of {len(self.masks)} masks not found.") | |||
| logging.getLogger().warning(f"{len(non_existing_masks)} of a total of {len(self.masks)} masks not found.") | |||
There was a problem hiding this comment.
depends on how we would like this function envolve, maybe consider initiate a logger to collect all the logs? Like you did in other modules.
There was a problem hiding this comment.
good catch! thanks!
(fixes #34)
see this pr's edits to the
readme.mdfor an overview of the added functionality