-
Notifications
You must be signed in to change notification settings - Fork 709
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
Add console logger #241
Add console logger #241
Conversation
…o feature/sa/add-console-logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just two minor comments.
tools/train.py
Outdated
@@ -39,6 +40,7 @@ def get_args() -> Namespace: | |||
parser = ArgumentParser() | |||
parser.add_argument("--model", type=str, default="padim", help="Name of the algorithm to train/test") | |||
parser.add_argument("--model_config_path", type=str, required=False, help="Path to a model config file") | |||
parser.add_argument("--log-level", type=str, help="<DEBUG, INFO, WARNING, ERROR>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this log-level
passed to the console logger somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, it's a bit more involved than we thought. It's something I was going to bring up. This could be either addressed in a separate PR, or this one needs to be modified a little bit.
anomalib/utils/loggers/__init__.py
Outdated
AVAILABLE_LOGGERS = ["tensorboard", "wandb", "csv"] | ||
|
||
|
||
class UnknownLogger(Exception): | ||
"""This is raised when the logger option in `config.yaml` file is set incorrectly.""" | ||
|
||
|
||
def get_logger( | ||
def get_console_logger(name: Optional[str] = None, level: Union[int, str] = logging.INFO) -> Logger: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could rename this configure_logger
and instead of having it return the logger, just define logger = logging.getLogger("anomalib")
in train.py
and the other entrypoitns. That way the logging in those entrypoint files would be more in line with the other submodules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would work if the existing logger
were renamed to something like experiment_logger
or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could rename the experiment logger to experiment_logger
. This would make sense since the function is already renamed to get_experiment_logger
in your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like the following in train.py
and other entrypoints?
...
configure_logger()
logger = logging.getLogger("anomalib")
...
If yes, this would cost us two lines for the sake of following the standard logging definition.
We could maybe rename this to get_logger
instead, which would be a one liner in this case. And the experiment logger would also be renamed accordingly.
...
logger = get_logger("anomalib", level=logging.INFO)
...
experiment_logger = get_experiment_logger(config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If yes, this would cost us two lines for the sake of following the standard logging definition.
It would add a single line logger = logging.getLogger("anomalib")
, but this line would be placed at the top of the file, right under the imports. So it may be an additional line, but it does not clutter the train
function.
Also this same line would be removed from the get_logger
/configure_logger
function, which would end up a lot cleaner, because it would also only require a single argument (log_level
), no logic for setting the logger name, and no return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
Add python console logger and replace print
Fixes Add proper logging and remove
print
statements withlogger
vialogger = logging.getLogger("pytorch_lightning")
#54Changes
Checklist