Skip to content

fix: merge user-supplied fit(callbacks=...) with built-in trainer callbacks - #469

Open
dripston wants to merge 1 commit into
OpenTabular:mainfrom
dripston:fix/452-trainer-callbacks-kwarg-collision
Open

dripston wants to merge 1 commit into
OpenTabular:mainfrom
dripston:fix/452-trainer-callbacks-kwarg-collision

Conversation

@dripston

Copy link
Copy Markdown

Problem

Calling fit(callbacks=[...]), exactly as docs/core_concepts/config_system.md and docs/core_concepts/training_and_evaluation.md document as a supported Lightning passthrough, crashes:

TypeError: lightning.pytorch.trainer.trainer.Trainer() got multiple values for keyword argument 'callbacks'

One of several independent defects filed together in #452 — this PR addresses only the callbacks= collision, since the others touch unrelated code paths (seeding, fit-failure state corruption, NODE initialization, predict(device=), profile(dry_run=True)).

Root cause

deeptab/models/_mixins/fit.py's pl.Trainer(...) construction hard-codes callbacks=[early_stop_callback, checkpoint_callback, ModelSummary(...)] and then spreads **trainer_kwargs after it. Any callbacks= in trainer_kwargs collides with the hard-coded keyword argument. logger= a few lines below already handles this correctly by popping the user's value out of trainer_kwargs and merging it in; callbacks had no equivalent handling, so there is no way to attach a custom callback (LR monitors, Optuna pruning, gradient-accumulation schedulers) despite the docs saying otherwise.

Fix

callbacks=[
    early_stop_callback,
    checkpoint_callback,
    ModelSummary(max_depth=2),
    *trainer_kwargs.pop("callbacks", []),
],

Mirrors the existing logger=trainer_kwargs.pop("logger", ...) pattern immediately below it. When no callbacks= is passed, behavior is unchanged (empty list unpacks to nothing). When one is passed, it's appended after the built-ins instead of colliding with them.

Testing

  • Added test_fit_accepts_user_supplied_callbacks in tests/test_models.py, following the existing MLPRegressor/regression_data/FIT_KWARGS fixtures used throughout that file. It fits with an explicit callbacks=[LearningRateMonitor()] and asserts both the user's callback and the built-in callbacks are present on model._trainer.callbacks.
  • python3 -m py_compile passes on both changed files.
  • Manually simulated the list-merge logic in isolation (pure Python, no Lightning dependency) against both the "user passes callbacks=" and "user passes nothing" cases — confirmed no collision and no change to default behavior.
  • Could not run the actual test suite in this environment (lightning/torch are not installed here) — please run pytest tests/test_models.py -k callbacks in CI/a configured dev environment to confirm.

Addresses #452 (callbacks portion only — not closing, since the issue bundles 6 additional unrelated defects)

🤖 Generated with Claude Code

…lbacks

pl.Trainer was constructed with a hard-coded callbacks=[...] list
followed by **trainer_kwargs, so an explicit fit(callbacks=[...])
collided and raised:

  TypeError: Trainer() got multiple values for keyword argument
  'callbacks'

The docs (config_system.md, training_and_evaluation.md) document
callbacks as a Lightning trainer kwarg forwarded from fit(), and
logger= already gets this override-merge treatment a few lines below
-- callbacks had no equivalent path, so there was no supported way to
attach a custom callback (LR logging, Optuna pruning, gradient
accumulation schedulers, etc).

Pop callbacks from trainer_kwargs and append it to the built-in
EarlyStopping/ModelCheckpoint/ModelSummary list, mirroring how logger
is already handled.

This addresses one of several independent defects filed together in
OpenTabular#452; the others (seed_context no-op, corrupted state after a failed
fit, NODE data-aware init leaking validation data, fit(random_state=)
being overridden by the constructor, predict(device=) being a no-op,
and profile(dry_run=True) not fully restoring state) are unrelated
code paths and are left to separate fixes.

Addresses OpenTabular#452 (callbacks portion only)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

1 participant