From fa243cf705ce79782fb132be7341a9b3ced36a5c Mon Sep 17 00:00:00 2001 From: dripston Date: Sun, 20 Sep 2026 00:19:41 +0530 Subject: [PATCH] fix: merge user-supplied fit(callbacks=...) with built-in trainer callbacks 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 #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 #452 (callbacks portion only) Co-Authored-By: Claude Sonnet 5 --- deeptab/models/_mixins/fit.py | 3 +++ tests/test_models.py | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/deeptab/models/_mixins/fit.py b/deeptab/models/_mixins/fit.py index 1f5eea1..6617711 100644 --- a/deeptab/models/_mixins/fit.py +++ b/deeptab/models/_mixins/fit.py @@ -495,10 +495,13 @@ def fit( self._trainer = pl.Trainer( max_epochs=max_epochs, + # Merge an explicit `callbacks=` in trainer_kwargs with our built-ins, + # the same way an explicit `logger=` overrides our default below. callbacks=[ early_stop_callback, checkpoint_callback, ModelSummary(max_depth=2), + *trainer_kwargs.pop("callbacks", []), ], # Let an explicit `logger=` in trainer_kwargs override our default. logger=trainer_kwargs.pop( diff --git a/tests/test_models.py b/tests/test_models.py index 0133277..1fd84ec 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -271,6 +271,27 @@ def test_regressor_fit_predict_shape(cls, regression_data): assert np.isfinite(preds).all(), f"{cls.__name__}.predict returned non-finite values" +def test_fit_accepts_user_supplied_callbacks(regression_data): + """fit(callbacks=[...]) must merge with the built-in callbacks, not collide with them. + + Regression test for https://github.com/OpenTabular/DeepTab/issues/452: `pl.Trainer` + was constructed with a hard-coded `callbacks=[...]` list followed by `**trainer_kwargs`, + so a user-supplied `callbacks=` raised `TypeError: ... got multiple values for keyword + argument 'callbacks'` even though the docs document `callbacks` as a Lightning + passthrough argument. + """ + from lightning.pytorch.callbacks import LearningRateMonitor + + X_train, _X_test, y_train, _y_test = regression_data + model = MLPRegressor() + lr_monitor = LearningRateMonitor() + model.fit(X_train, y_train, callbacks=[lr_monitor], **FIT_KWARGS) + + assert lr_monitor in model._trainer.callbacks + # Built-in callbacks (EarlyStopping, ModelCheckpoint, ModelSummary) must still be present. + assert len(model._trainer.callbacks) >= 4 + + @pytest.mark.parametrize("cls", REGRESSORS) def test_regressor_evaluate_returns_dict(cls, regression_data): X_train, X_test, y_train, y_test = regression_data