Not looked at code yet, but from your description, I'd be worried that you are not maintaining access to fitted steps with steps, get_params, set_params and that it might not be possible to construct a pipeline from a list of fitted steps. All of these behaviours must remain during deprecation.

I made the changes.

We also must provide an official way to construct a pipeline from fitted components (e.g. a substring of a fitted pipeline).

@jnothman was proposing a get_steps().

Any suggestions @jnothman @ogrisel @raghavrv @GaelVaroquaux @agramfort ?

Codecov Report

Merging #8350 into master will increase coverage by 0.74%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8350      +/-   ##
==========================================
+ Coverage   94.75%   95.49%   +0.74%     
==========================================
  Files         342      342              
  Lines       60801    61026     +225     
==========================================
+ Hits        57609    58274     +665     
+ Misses       3192     2752     -440

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8694278...c30b8ff. Read the comment docs.

Do you need to construct it in the official scikit-learn way, ie from a
the constructor, or is it enough to instantiate the Pipeline and modify
the .steps_ afterwards?

Other estimated being constructed with existing models are done by modifying coef_ or whatever. But it's not clear to what extent this is recommended. For instance, when replacing an attribute in __dict__ with a property, we've rarely bothered implementing setters. When adding an attribute in version x, we've not checked that the user setting only the attributes available in version x-1 will suffice to not get a NotFittedError. Basically, we've given the user no assurances that this is okay, let alone recommended it.

I often find myself wanting to slice up pipelines. It deserves both how-tos and tests. Hacking steps_ is probably enough, but we should write out what that looks like and see if we're happy with it.

If it's not enough, I support the addition of a "Frozen" class that makes
an estimator robust to cloning

This either requires clone to be aware of the specific frozen case, or for clone to more generally support polymorphism or singledispatch. I'm still in favour of the latter -- and given this change have an ancient implementation of a wrapper for a fitted estimator -- though I know you feel it opens floodgates to misuse and error.

as well as a "freeze" function that would apply this to a list of estimator.

as in copy/wrap the estimators? or mutate them to mark them frozen?

I do think that the ability to freeze things is valuable all over the place, notably for semi-supervision (e.g. decomposition transformation trained on lots of unlabelled data incorporated in feature processing pipeline for supervised task).

Thinking about this instead of sleeping. Cloning is only part of the problem with freezing. The other part is overwriting fit so it does nothing. This can only be generically achieved with a wrapper.

• clone adds is isinstance(obj, FrozenModel): return obj.
• FrozenModel has fit(self, *args, **kwargs): return self
• FrozenModel delegates __getattr__ and __setattr__ to its wrapped estimator.
  • hence its estimator cannot be accessible at FrozenModel().estimator but at some more munged name.
• freeze(obj, copy=True)
  • if copy, performs deepcopy
  • if an estimator, returns FrozenModel(obj)
  • if a list or object array, returns list(map(FrozenModel, obj))
• thus Pipeline(freeze(my_pipeline.steps_[:-1])).fit(X, y).transform(X) sort of does what Pipeline(my_pipeline.steps[:-1]).transform(X) used to.
  • The need to call fit outside of a CV context or similar where it's done for you is a bit annoying. This still may not remain the nicest interface for Pipeline substring extraction.

(I've made some edits where I'd failed to fix up a previous version of that comment)

Not tonight :p

I vomited my panicked thoughts about freezing into issue #8370.

@jnothman @GaelVaroquaux Shall I change [WIP] to [MRG] get some reviews.
Any potential merging of this PR should wait for the introduction of the 'freezing business', right?

What is the desired behavior of cloning a compose.Pipeline when the final step sets warm_start=True?

You don't want to clone the final estimator and use the one passed in estimator.

I have a few questions:

1. It seems that Joel's concern about being able to build pipelines out of other pipelines addressed by the new slicing mechanism. Is that correct or am I missing something?
2. Why is warm_start + cloning a problem with pipelines? Cloning an estimator resets the state of the estimator (regardless of warm-start), why should it be any different for a pipeline??
3. What is stalling the PR now? Is there anything I can help with to move this forward?

cc @thomasjpfan

Thanks @jnothman .

We discussed it with @thomasjpfan and we thought about the following solution:

1. instead of always cloning the estimators in fit, only clone them the first time fit is called (with e.g. self.steps_ = getattr(self, 'steps_', [clone(est) for est in self.steps]))
   This way you can still do stuff like

   pipe = make_pipeline(...)
   pipe.fit(...)
   pip.set_param(gbdt__max_iter=100, gbdt__warm_start=True)
   pip.fit(...)  # warm start works ok

2. in __init__, set self.steps = [clone(est) for est in steps]. This is not breaking our API convention, since self.steps still behaves exactly like the steps parameter.
   We think this is needed because it prevents unexpected behaviors/bugs. If we don't clone, changing the pipeline with set_params will change an estimator passed as parameter, and changing an estimator parameter will also changes the pipeline (we want to avoid that).

Yes, set_paramas would keep both steps and steps_ synchronized.

And I can't say I like the idea of not fitting from scratch in Pipeline by default, but I can't quite put my finger on a specific problem with it.

It is still fitting from scratch as long as the items in steps follow our API. The nice thing is that if one of the steps has warm_start=True, this also works. Pipeline is delegating what it means to fit to steps_.

For behaviour (2), rather than cloning in init, you would need to clone
every time a step parameter (or steps) was set, if I'm not mistaken.

Agreed. It can be a @property with a setter, where it is initially set at __init__.

And I can't say
I like the idea of not fitting from scratch in Pipeline by default, but I
can't quite put my finger on a specific problem with it.

I feel like we always want to fit a pipeline from scratch? Not fitting from scratch is an extreme use case that we don't want to support (at least explicitly?). It means that your final estimator has been already fitted on whatever the pre-processors of your pipeline can output... How do you do that without the pipeline in the first place?

For behaviour (2), rather than cloning in init, you would need to clone
every time a step parameter (or steps) was set, if I'm not mistaken.

You mean someone doing e.g. pipe.steps[2] = some_new_est? Shouldn't this be totally forbidden? users should be using set_params

Shouldn't this be totally forbidden? users should be using set_params

pipe = Pipeline(
    [('pre', StandardScaler()),
    ('est', LogisticRegression())]
)
pipe.set_params(est=MLPClassifier())

yeah so we can just clone in set_params too

Question: how bad is the problem we are trying to solve here? Do we really care that pipelines don't respect our API conventions? Does it prevent us from doing more cool things?

I can't say I have a compelling reason to go through all this, and it seems that the original motivation #8157 was addressed already.

But if we do I'd like to move forward and try to implement #8350 (comment).

I agree the current behaviour is mostly innocuous and have never thought the change was a priority without checking all the use cases are going to be handled.

We have had bugs raised where the fundamental issue was the non cloning in Pipeline.... But I can't find them. And they're more "surprising" than "broken".

The other reason to do this is to set an example of how to build such things in a compliant way!

So... should we remove it from the roadmap?

actually, this issue is quite weird/annoying and is caused by this: #10063

We could also try and fix it by cloning the params in GridSearchCV but that would be somewhat strange as well...

removing milestone, I think this needs a slep or a close.

We can certainly have another look at this, but I think we can close this one and a fresh PR at some point would make more sense.

Yep, it will be scikit-learn 2.0 anyway.