[BUG] `clone` does not clone config for nested objects

[tpvasconcelos]

Describe the bug

BaseObject clone does not clone the config of nested BaseObjects.

The issue popped up when I was trying to set a {"backend:parallel": "joblib"} config to a forecaster but the config was getting removed when a wrapper pipeline object would get cloned (which seems to happen at various places throughout the sktime project)

To Reproduce

from sktime.forecasting.compose import TransformedTargetForecaster
from sktime.forecasting.naive import NaiveForecaster

pipeline = TransformedTargetForecaster(
    [("forecaster", NaiveForecaster().set_config(foo="bar"))]
)

assert pipeline.forecaster_.get_config()["foo"] == "bar"

pipeline_clone = pipeline.clone()

assert pipeline_clone == pipeline

pipeline_clone.forecaster_.get_config()["foo"] == "bar"
# Traceback (most recent call last):
# ...
# KeyError: 'foo'

Expected behavior

Expected all configs from nested objects to get cloned on .clone()

Additional context

The fix would be an extension to the work done in #257

If you agree, I'd be happy to contribute the fix to the skbase project. For reference, I opened this PR with a proposed patch ---> #275

Versions

Details

System:
python: 3.10.13 (main, Aug 24 2023, 12:59:26) [Clang 15.0.0 (clang-1500.0.40.1)]
executable: .venv/bin/python
machine: macOS-14.2.1-arm64-arm-64bit
Python dependencies:
pip: 23.3.1
sktime: 0.26.0
sklearn: 1.4.0
skbase: 0.7.1
numpy: 1.26.3
scipy: 1.12.0
pandas: 2.1.4
matplotlib: 3.8.2
joblib: 1.3.2
numba: 0.58.1
statsmodels: 0.14.1
pmdarima: 2.0.4
statsforecast: 1.7.1
tsfresh: None
tslearn: None
torch: None
tensorflow: None
tensorflow_probability: None

[fkiraly]

That's good catch, and you are very quick with the fix!

I will move this issue to skbase where the bug is.

[fkiraly]

Btw, I agree that nested configs should also get cloned, otherwise estimator configs are not too useful in common nested cases.

[fkiraly]

here's a question: should there be a deep arg to get_config, like get_params? That way, we could avoid changes to _clone, as then we just need to add the deep arg in clone, once. It would be "cleaner" imo since it completely decouples the param logic from the config logic (coupling/cohesion improvement).

However, I would consider this worth thinking about for another PR, rather than a requirement to change in the current PR - I would guess that changing this might be much more fiddly, as on needs to introduce the entire logic to deal with the double-underscore convention etc.

[tpvasconcelos]

here's a question: should there be a deep arg to get_config, like get_params? That way, we could avoid changes to _clone, as then we just need to add the deep arg in clone, once. It would be "cleaner" imo since it completely decouples the param logic from the config logic (coupling/cohesion improvement).

However, I would consider this worth thinking about for another PR, rather than a requirement to change in the current PR - I would guess that changing this might be much more fiddly, as on needs to introduce the entire logic to deal with the double-underscore convention etc.

Further, non-blocking question, I was wondering: should we make the contract less strict about inheritance, and more about interfaces, would it make sense to do hasattr(param, "clone") and hasattr(param, "_config")? Or perhaps it is safer to make the deviation from sklearn more strict, which is the current state. I tend towards the latter.

I don't have a strong opinion here but I can't see why someone wouldn't want to clone the config of an object without cloning the config of "nested" objects. Maybe I'm missing something?

[fkiraly]

I can't see why someone wouldn't want to clone the config of an object without cloning the config of "nested" objects. Maybe I'm missing something?

I think you are right about that, but the question above is slightly different - namely, should get_config, set_config of the top node object support getting/setting of component node configs - like get_params, set_params - or do you have to address the components directly.

I.e., compare

# how it would work right now
composite = Composite(comp1=Component1(), comp2=Component2().set_config(foo="bar"))
# vs how it could also work
composite = Composite(comp1=Component1(), comp2=Component2())
composite.set_config(comp2__foo="bar")

Both ways are already making the assumption that clone should keep configs across all nodes.

[tpvasconcelos]

I didn't properly read your comment initially. I see what you mean now. Your suggestion makes sense to me I think