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

Restored metric logging to third-party loggers #2489

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AdityaSinghDevs
Copy link

📝 Description

  • External Logger Integration: Added support for logging to external services (Comet, Wandb, MLFlow, TensorBoard).
  • Logger Initialization: Introduced _initialize_loggers to configure and initialize loggers.
  • Logging Metrics: Metrics are now logged to external services after benchmark completion.
  • Error Handling: Added handling for missing logger modules with a warning to install required packages.
  • 🛠️ Fixes [Task]: Restore metric logging to third-party loggers in Benchmarking Pipeline #2056

✨ Changes

Select what type of change your PR is:

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🔨 Refactor (non-breaking change which refactors the code base)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🔒 Security update

✅ Checklist

Before you submit your pull request, please make sure you have completed the following steps:

  • 📋 I have summarized my changes in the CHANGELOG and followed the guidelines for my type of change (skip for minor changes, documentation updates, and test enhancements).
  • 📚 I have made the necessary updates to the documentation (if applicable).
  • 🧪 I have written tests that support my changes and prove that my fix is effective or my feature works (if applicable).

For more information about code review checklists, see the Code Review Checklist.

@AdityaSinghDevs
Copy link
Author

Hi @ashwinvaidya17 , @samet-akcay ,
Just checking in on this PR. Let me know if you need any changes or feedback.
Would appreciate a review when you get a chance. Thanks!

Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few questions.

src/anomalib/pipelines/benchmark/job.py Outdated Show resolved Hide resolved
src/anomalib/pipelines/benchmark/job.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 left a comment

Choose a reason for hiding this comment

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

Try to avoid multiple lines of try/except for imports. I've added a few lines of code as suggestion. Feel free to use them as a reference for refactoring.
Additionally, I would recommend manually testing whether the script runs without any backends (mlflow, tensorboard, etc) in your environment, and when some of them are present. There might be some edge cases here

@@ -89,8 +125,44 @@ def run(
**test_results[0],
}
logger.info(f"Completed with result {output}")
# Logging metrics to External Loggers (excluding TensorBoard)
trainer = engine.trainer()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure that you can call trainer?

Comment on lines +26 to +60
# Import external loggers
AVAILABLE_LOGGERS: dict[str, Any] = {}

try:
from anomalib.loggers import AnomalibCometLogger

AVAILABLE_LOGGERS["comet"] = AnomalibCometLogger
except ImportError:
logger.debug("Comet logger not available. Install using `pip install comet-ml`")
try:
from anomalib.loggers import AnomalibMLFlowLogger

AVAILABLE_LOGGERS["mlflow"] = AnomalibMLFlowLogger
except ImportError:
logger.debug("MLflow logger not available. Install using `pip install mlflow`")
try:
from anomalib.loggers import AnomalibTensorBoardLogger

AVAILABLE_LOGGERS["tensorboard"] = AnomalibTensorBoardLogger
except ImportError:
logger.debug("TensorBoard logger not available. Install using `pip install tensorboard`")
try:
from anomalib.loggers import AnomalibWandbLogger

AVAILABLE_LOGGERS["wandb"] = AnomalibWandbLogger
except ImportError:
logger.debug("Weights & Biases logger not available. Install using `pip install wandb`")

LOGGERS_AVAILABLE = len(AVAILABLE_LOGGERS) > 0

if LOGGERS_AVAILABLE:
logger.info(f"Available loggers: {', '.join(AVAILABLE_LOGGERS.keys())}")
else:
logger.warning("No external loggers available. Install required packages using `anomalib install -v`")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Import external loggers
AVAILABLE_LOGGERS: dict[str, Any] = {}
try:
from anomalib.loggers import AnomalibCometLogger
AVAILABLE_LOGGERS["comet"] = AnomalibCometLogger
except ImportError:
logger.debug("Comet logger not available. Install using `pip install comet-ml`")
try:
from anomalib.loggers import AnomalibMLFlowLogger
AVAILABLE_LOGGERS["mlflow"] = AnomalibMLFlowLogger
except ImportError:
logger.debug("MLflow logger not available. Install using `pip install mlflow`")
try:
from anomalib.loggers import AnomalibTensorBoardLogger
AVAILABLE_LOGGERS["tensorboard"] = AnomalibTensorBoardLogger
except ImportError:
logger.debug("TensorBoard logger not available. Install using `pip install tensorboard`")
try:
from anomalib.loggers import AnomalibWandbLogger
AVAILABLE_LOGGERS["wandb"] = AnomalibWandbLogger
except ImportError:
logger.debug("Weights & Biases logger not available. Install using `pip install wandb`")
LOGGERS_AVAILABLE = len(AVAILABLE_LOGGERS) > 0
if LOGGERS_AVAILABLE:
logger.info(f"Available loggers: {', '.join(AVAILABLE_LOGGERS.keys())}")
else:
logger.warning("No external loggers available. Install required packages using `anomalib install -v`")
def try_create_logger(logger_class: str, config: dict[str, dict[str, Any]]) -> Logger:
"""Try to import a logger class.
Args:
logger_class (str): The name of the logger class to import.
config (dict[str, dict[str, Any]]): The configuration for the logger.
Returns:
Logger: The logger instance.
"""
try:
module = importlib.import_module("anomalib.loggers")
logger_class = getattr(module, f"Anomalib{logger_class}Logger")
return logger_class(**config)
except (ImportError, ModuleNotFoundError):
logger.info(
f"{logger_class} logger not available. Please install the respective package.",
)
return None

@@ -69,6 +104,7 @@ def run(
accelerator=self.accelerator,
devices=devices,
default_root_dir=temp_dir,
logger=self._initialize_loggers(self.flat_cfg or {}) if LOGGERS_AVAILABLE else [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger=self._initialize_loggers(self.flat_cfg or {}) if LOGGERS_AVAILABLE else [],
logger=self._initialize_loggers(self.flat_cfg),

Comment on lines +128 to +136
# Logging metrics to External Loggers (excluding TensorBoard)
trainer = engine.trainer()
for logger_instance in trainer.loggers:
if any(
isinstance(logger_instance, AVAILABLE_LOGGERS.get(name, object))
for name in ["comet", "wandb", "mlflow"]
):
logger_instance.log_metrics(test_results[0])
logger.debug(f"Successfully logged metrics to {logger_instance.__class__.__name__}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Logging metrics to External Loggers (excluding TensorBoard)
trainer = engine.trainer()
for logger_instance in trainer.loggers:
if any(
isinstance(logger_instance, AVAILABLE_LOGGERS.get(name, object))
for name in ["comet", "wandb", "mlflow"]
):
logger_instance.log_metrics(test_results[0])
logger.debug(f"Successfully logged metrics to {logger_instance.__class__.__name__}")
# Logging metrics to External Loggers (excluding TensorBoard)
for logger_instance in engine.trainer.loggers:
logger_instance.log_metrics(test_results[0])
logger.info(f"Successfully logged metrics to {logger_instance.__class__.__name__}")

Comment on lines +139 to +164
@staticmethod
def _initialize_loggers(logger_configs: dict[str, dict[str, Any]]) -> list[Any]:
"""Initialize configured external loggers.

Args:
logger_configs: Dictionary mapping logger names to their configurations.

Returns:
List of initialized loggers.
"""
active_loggers = []
default_configs = {
"tensorboard": {"save_dir": "logs/benchmarks"},
"comet": {"project_name": "anomalib"},
"wandb": {"project": "anomalib"},
"mlflow": {"experiment_name": "anomalib"},
}

for logger_name, logger_class in AVAILABLE_LOGGERS.items():
# Use provided config or fall back to defaults
config = logger_configs.get(logger_name, default_configs.get(logger_name, {}))
logger_instance = logger_class(**config)
active_loggers.append(logger_instance)
logger.info(f"Successfully initialized {logger_name} logger")

return active_loggers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@staticmethod
def _initialize_loggers(logger_configs: dict[str, dict[str, Any]]) -> list[Any]:
"""Initialize configured external loggers.
Args:
logger_configs: Dictionary mapping logger names to their configurations.
Returns:
List of initialized loggers.
"""
active_loggers = []
default_configs = {
"tensorboard": {"save_dir": "logs/benchmarks"},
"comet": {"project_name": "anomalib"},
"wandb": {"project": "anomalib"},
"mlflow": {"experiment_name": "anomalib"},
}
for logger_name, logger_class in AVAILABLE_LOGGERS.items():
# Use provided config or fall back to defaults
config = logger_configs.get(logger_name, default_configs.get(logger_name, {}))
logger_instance = logger_class(**config)
active_loggers.append(logger_instance)
logger.info(f"Successfully initialized {logger_name} logger")
return active_loggers
@staticmethod
def _initialize_loggers(training_config: dict[str, dict[str, Any]]) -> list[Logger]:
"""Initialize configured external loggers.
Args:
training_config: Dictionary mapping logger names to their configurations.
Returns:
List of initialized loggers.
"""
active_loggers: list[Logger] = []
default_configs = {
"TensorBoard": {"save_dir": "logs/benchmarks"},
"Comet": {"project_name": "anomalib"},
"Wandb": {"project": "anomalib"},
"MLFlow": {"experiment_name": "anomalib"},
}
for logger_name, default_config in default_configs.items():
# Use provided config or fall back to defaults
config = training_config.get(logger_name.lower(), default_config)
logger_instance = try_create_logger(logger_name, config)
if logger_instance is None:
continue
active_loggers.append(logger_instance)
logger.info(f"Successfully initialized {logger_name} logger")
return active_loggers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Restore metric logging to third-party loggers in Benchmarking Pipeline
3 participants