fix(emissions_tracker): warn and skip LOGGER output when logging_logger is missing (#1412)

[Prasanthmax]

Closes #1412

Root cause

_init_output_methods() appended self._logging_logger to the handlers list
when OutputMethod.LOGGER was requested, even when it was None. The
resulting AttributeError was swallowed by a suppress() block in
_persist_data(), so tracker.stop() silently produced no output.

Fix

When OutputMethod.LOGGER is requested without a logging_logger, log a
warning and skip the handler. The tracker keeps its other outputs.

My first version raised a ValueError in the constructor. @davidberenstein1957
pointed out in review that this would break users who enable save_to_logger
through the config file or an environment variable: they cannot supply a
logger object from there, so a raise would turn a setting that works today
into a crash. Warning and skipping covers that case.

Testing

• Replaced the raising test with
  test_logger_output_warns_and_skips_without_logging_logger, which checks
  the warning and that no None ends up in _output_handlers.
• Kept test_logger_output_succeeds_with_logging_logger.
• Reproduced the config/env case by hand: with CODECARBON_SAVE_TO_LOGGER=true
  and no logger passed, the tracker is created, warns, and keeps CSV output.
• test_emissions_tracker.py and test_custom_handler.py pass locally. The
  test_custom_handler.py change from the first version was reverted.

Thanks @sohammishra864-wq for reporting this.

[davidberenstein1957]

Thanks, the test and the test_custom_handler.py update are good. One design point before this merges: raising ValueError in the constructor is a breaking change for anyone who sets save_to_logger=True from the environment or config file without passing logging_logger. Today they get a silently missing handler, with this PR they get a crash at tracker creation.

I would rather log a warning and skip the LOGGER handler, so the tracker still records to its other outputs. Could you change the raise to a warning and adjust the test?

[Prasanthmax]

Hi @davidberenstein1957, thanks for the review, and for flagging this. You’re right. I tested the explicit constructor path but not users who enable save_to_logger through the config file or an env var. They can’t pass a logging_logger object from there, so raising would break setups that work today. I’ll switch the raise to a warning that skips the LOGGER handler, update the tests, and revert my edits to test_decorator_flush and test_save_to_flags_map_to_output_methods_and_warn. Will push shortly.