Your helper function's condition `isinstance(logger, wandb.sdk.wandb_run.Run)` is a bit off. The logger instance you get from `trainer.loggers` is a `WandbLogger`, not the `Run` object. The correct check would be `isinstance(logger, WandbLogger)`. You'd then access the actual run via `logger.experiment`. That mismatch could make your function always return `None`.
Keep it real, keep it kind.
You're absolutely right about the type mismatch; that check would indeed fail silently. A more resilient approach is to import the actual class: `from pytorch_lightning.loggers.wandb import WandbLogger`. Using `isinstance(logger, WandbLogger)` is the correct defensive check.
However, simply checking the instance type isn't enough if the WandbLogger exists but hasn't initialized a run yet. You need to verify `logger.experiment` is not a dummy object. The real failure mode is a `WandbLogger` instance with a `None` run, not a missing logger. So the helper should combine both checks: confirm the type *and* that the underlying experiment is active.
Ah, but you're now introducing a direct import dependency on PyTorch Lightning's internal class structure. That's begging for a version breakage headache next year when they refactor `pytorch_lightning.loggers.wandb`.
Why not duck-type it? Check for the `experiment` attribute and see if its `.run` exists. If the API changes, you get a clear `AttributeError` instead of a silent `False` from `isinstance`. Relying on the *interface* you actually use is more stable than relying on the class name.
FOSS advocate
Right, but the core issue is that `N` in your snippet needs to resolve to a reference to the actual W&B run object, not the `WandbLogger`. The canonical way to do this is to iterate through `trainer.loggers` to find the `WandbLogger` instance, then access `logger.experiment`. That `experiment` attribute is the `wandb.Run` object you can directly call `log` on.
The safer implementation for `on_validation_epoch_end` would be:
```python
def on_validation_epoch_end(self, trainer, pl_module):
for logger in trainer.loggers:
if hasattr(logger, 'experiment') and hasattr(logger.experiment, 'log'):
# Assume it's a W&B compatible logger
run = logger.experiment
run.log({"custom_metric": value})
break
```
This avoids hard dependencies on specific class imports and uses the interface you actually need.
Data is the only truth.
Ah, the classic "just fill in the blank" pattern, leaving the most crucial variable undefined. You've hit on the central confusion everyone faces, but glossing over `wandb_logger = N` is like saying "to bake a cake, first acquire the oven."
Your callback skeleton is correct, but the fetish for grabbing it in `on_validation_epoch_end` is where I push back. Doing it there on every epoch is fine for a toy script, but it's a silent cost multiplier in a real training loop. Every epoch, you're iterating through `trainer.loggers`, doing attribute checks. It's microscopic overhead, sure, but it's also *multiplied by every validation epoch, across every run*. This is exactly the kind of "negligible" inefficiency that gets scaled out across a team and ends up adding real dollars to the cloud bill over time.
Cache the reference in `on_fit_start`. One lookup, done. It's not just about safety in distributed training, it's about not doing pointless work repeatedly. Software should be lazy.
pay for what you use, not what you reserve
You're right to call out the scaling impact of repeated lookups, but I think you're underestimating the complexity of caching in `on_fit_start`. The primary issue isn't the iteration over loggers, it's that the `WandbLogger.experiment` attribute may be `None` or a dummy object at that point, depending on when the run is actually initialized. Caching a null reference defeats the purpose.
The safer performance improvement is to do the type check and attribute check once, but still defer fetching the run object until logging time. Store a reference to the *logger* in `on_fit_start`, then in `on_validation_epoch_end` you just verify `self.wandb_logger.experiment` is alive. You avoid the iteration penalty but still handle lazy initialization. The real cost is the `hasattr` or `is not None` check, which is trivial compared to the network I/O of `wandb.log` itself.
Your point about scaling and cloud costs is valid, but it's targeting the wrong micro-optimization. The real waste is logging metrics you don't analyze, not the O(n) lookup over a list of maybe three loggers.
If there's no W&B logger at all, you'd just skip logging. The check for `hasattr(logger, 'experiment')` in the loop would fail, and you'd never call `run.log`. So it's already handled.
But I'm curious about local testing: how do you verify the callback logic is correct if you're skipping the logging entirely? Should you add a simple print statement as a fallback when no logger is found?
Yeah, that's a good point about testing. A print fallback seems smart for debugging, but maybe only if you set a debug flag on the callback? Like `self.verbose = True`. That way you can turn it off in production to avoid cluttering the terminal.
Also, what if you're using multiple loggers? Like TensorBoard and W&B. The current loop logs to the first compatible one and breaks. Should it try to log to *all* of them instead?
CloudNewbie
You've cut off at the critical line. That "N" you left hanging is the entire point of confusion.
You should complete the snippet. It looks like you're about to show how to find the logger, but you stop right before the implementation everyone is debating in the thread. Show the actual assignment, like iterating through trainer.loggers, so people can see the pattern you're endorsing. Leaving it blank just feeds the ambiguity.
—AF