[MRG] Avoid nonlinear complexity for repr of nested estimators when print_c… #18508

Xethan · 2020-10-01T18:21:09Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

When print_changed_only is True and repr is called on an estimator, parameters are checked to see if they are different from the default value using repr. This causes a complexity issue for nested estimators because it will recursively call repr on the attributes that are estimators and so on, so we first check if the parameter is an estimator with a type different than the default value (often the parameter will have no default value) and exit the if early.

I tested it on the case that caused me trouble and it went from 30+ minutes to roughly 1 minute.

Any other comments?

I don't know if it is possible to do a non-regression test as functionality is unchanged. I don't know how to use asv benchmarks and I would need to reproduce the issue with base estimators first.

…hanged_only=True

thomasjpfan · 2020-10-01T22:29:11Z

I tested it on the case that caused me trouble and it went from 30+ minutes to roughly 1 minute.

As a reference, what is the runtime when print_changed_only=False in your use case?

Xethan · 2020-10-02T11:14:41Z

With print_changed_only=False, the runtime is ~2 seconds.
I have tried investigating further with no success for now.

Xethan · 2020-10-02T12:45:35Z

I found the remaining problem. For Pipeline objects, the attribute steps is not filtered by the condition I added and thus repr is called on steps and the estimators it contains.

Xethan · 2020-10-02T13:02:27Z

To fix it, we can either put an additional check specific to pipelines (or iterators of estimators) but it feels quite ugly, maybe parameters that have no default value should just be automatically considered as changed ? It is indirect but cleaner / simpler.

NicolasHug · 2020-10-02T13:20:52Z

parameters that have no default value should just be automatically considered as changed ?

That makes sense yes. We can compare the default with inspect._empty.

NicolasHug

Does the new check with _empty help with printing time @Xethan ?

sklearn/utils/_pprint.py

NicolasHug

Thanks for you work @Xethan
This looks good.

I think we should add a short test in sklearn/utils/tests/test_pprint.py in order to prevent any regression in the future. The test could just count the number of calls to repr as you've previously done (I believe). LMK if you need any guidance.

Since this provides a significant improvement we should also write an Enhancement entry in doc/whats_new/v0.24.rst. Make sure to credit yourself and mention this PR, like other entries there.

Thanks!

sklearn/utils/_pprint.py

NicolasHug · 2020-10-02T14:15:54Z

sklearn/utils/_pprint.py

+            return True
+        if init_params[k] == inspect._empty:  # k has no default value
+            return True
+        # avoid `repr` on nested estimators, it causes nonlinear complexity


Suggested change

# avoid `repr` on nested estimators, it causes nonlinear complexity

# try to avoid calling repr on nested estimators

I added the test and verified it fails as expected when any of the two fixes is commented out.

NicolasHug

Thanks @Xethan , some last nits but LGTM!

NicolasHug · 2020-10-02T16:13:32Z

sklearn/utils/tests/test_pprint.py

+        def transform(self, X, copy=None):
+            return self


We don't need this because we never call fit on the pipeline

When transform was not there, I had this error :

E TypeError: All intermediate steps should be transformers and implement fit and transform or be the string 'passthrough' 'DummyEstimator(estimator=DummyEstimator())' (type <class 'sklearn.utils.tests.test_pprint.test_complexity_print_changed_only.<locals>.DummyEstimator'>) doesn't

I am also going to add estimator.transform(None) because a coverage test has failed due to this function not being called.

It's preferable to just add # pragma: no cover instead

NicolasHug · 2020-10-02T16:17:30Z

doc/whats_new/v0.24.rst

+- |Enhancement| Avoid nonlinear complexity for ``repr`` of nested estimators
+  when `print_changed_only=True`.


I think we should avoid the term nonlinear complexity and instead be very explicit that the calls are now faster.

Also, this should be instead in a Miscellaneous section as done in doc/whats_new/v0.23.rst

Suggested change

- |Enhancement| Avoid nonlinear complexity for ``repr`` of nested estimators

when `print_changed_only=True`.

- |Enhancement| Calls to ``repr`` are now faster when `print_changed_only=True`, especially with meta-estimators.

NicolasHug · 2020-10-02T16:18:12Z

sklearn/utils/tests/test_pprint.py

+    class ReprCounter(BaseEstimator):
+        nb_times_repr_called = 0
+        def __repr__(self):
+            ReprCounter.nb_times_repr_called += 1
+            return super().__repr__()


Instead of defining this base class, would it be equivalent to just have nb_times_repr_called as a global variable and DummyEstimator.__repr__ to be the current __repr__ of ReprCounter?

thomasjpfan

Thank you for the PR @Xethan

Minor comment, otherwise LGTM!

thomasjpfan · 2020-10-02T18:36:15Z

sklearn/utils/tests/test_pprint.py

+            return super().__repr__()
+
+        def transform(self, X, copy=None):  # pragma: no cover
+            return self


Nit:

Suggested change

return self

return X

jnothman

looking good!

jnothman · 2020-10-04T23:29:24Z

sklearn/utils/_pprint.py

+        if init_params[k] == inspect._empty:  # k has no default value
+            return True
+        # try to avoid calling repr on nested estimators
+        if (isinstance(v, BaseEstimator) and


you could check if either v or init_params[k] is an estimator, in fact

Is there code using estimators as default value ? I have never seen any and I doubt it would be nested estimators but I can add the check if you think it's useful.

I have seen places (maybe not in sklearn itself) where the default value is an estimator, but then v in those cases is also an estimator. And since this is only a minor performance issue, I'm fine as is.

jnothman · 2020-10-05T20:37:04Z

Yes, you are right, it would not be recommended to use a mutable default value

Xethan · 2020-10-06T14:55:44Z

Is there anything left to do then, or can it be merged ?

adrinjalali

Thanks @Xethan

adrinjalali · 2020-10-08T12:48:10Z

sklearn/utils/_pprint.py

+        if init_params[k] == inspect._empty:  # k has no default value
+            return True
+        # try to avoid calling repr on nested estimators
+        if (isinstance(v, BaseEstimator) and


I have seen places (maybe not in sklearn itself) where the default value is an estimator, but then v in those cases is also an estimator. And since this is only a minor performance issue, I'm fine as is.

…nt_c… (scikit-learn#18508) * Avoid nonlinear complexity for repr of nested estimators when print_changed_only=True * Automatically consider parameters with no default value as changed * Use helper function for improved readability * Add a non-regression test and a release note entry Co-authored-by: nathan <nathan@sancare.fr>

Avoid nonlinear complexity for repr of nested estimators when print_c…

e97e0a7

…hanged_only=True

github-actions bot added the module:utils label Oct 1, 2020

Automatically consider parameters with no default value as changed

ed6480e

NicolasHug reviewed Oct 2, 2020

View reviewed changes

sklearn/utils/_pprint.py Outdated Show resolved Hide resolved

Use helper function for improved readability

ea96574

NicolasHug reviewed Oct 2, 2020

View reviewed changes

NicolasHug approved these changes Oct 2, 2020

View reviewed changes

Xethan force-pushed the avoid_nonlinear_complexity_for_repr_of_nested_estimators branch 2 times, most recently from 42ddb05 to aa4d02c Compare October 2, 2020 16:41

thomasjpfan approved these changes Oct 2, 2020

View reviewed changes

Add a non-regression test and a release note entry

102433c

Xethan force-pushed the avoid_nonlinear_complexity_for_repr_of_nested_estimators branch from aa4d02c to 102433c Compare October 3, 2020 16:09

jnothman reviewed Oct 4, 2020

View reviewed changes

adrinjalali approved these changes Oct 8, 2020

View reviewed changes

adrinjalali merged commit cdff9b0 into scikit-learn:master Oct 8, 2020
5 checks passed

[MRG] Avoid nonlinear complexity for repr of nested estimators when print_c… #18508

[MRG] Avoid nonlinear complexity for repr of nested estimators when print_c… #18508

Xethan commented Oct 1, 2020

thomasjpfan commented Oct 1, 2020

Xethan commented Oct 2, 2020

Xethan commented Oct 2, 2020 •

edited

Xethan commented Oct 2, 2020

NicolasHug commented Oct 2, 2020

NicolasHug left a comment

NicolasHug left a comment

NicolasHug Oct 2, 2020

Xethan Oct 2, 2020

NicolasHug left a comment

NicolasHug Oct 2, 2020

Xethan Oct 2, 2020

Xethan Oct 2, 2020 •

edited

NicolasHug Oct 2, 2020

NicolasHug Oct 2, 2020

NicolasHug Oct 2, 2020

thomasjpfan left a comment

thomasjpfan Oct 2, 2020

jnothman left a comment

jnothman Oct 4, 2020

Xethan Oct 5, 2020

adrinjalali Oct 8, 2020

jnothman commented Oct 5, 2020 via email

Xethan commented Oct 6, 2020

adrinjalali left a comment

adrinjalali Oct 8, 2020

	# avoid `repr` on nested estimators, it causes nonlinear complexity
	# try to avoid calling repr on nested estimators

		- \|Enhancement\| Avoid nonlinear complexity for ``repr`` of nested estimators
		when `print_changed_only=True`.

	- \|Enhancement\| Avoid nonlinear complexity for ``repr`` of nested estimators
	when `print_changed_only=True`.
	- \|Enhancement\| Calls to ``repr`` are now faster when `print_changed_only=True`, especially with meta-estimators.

[MRG] Avoid nonlinear complexity for repr of nested estimators when print_c… #18508

[MRG] Avoid nonlinear complexity for repr of nested estimators when print_c… #18508

Conversation

Xethan commented Oct 1, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

thomasjpfan commented Oct 1, 2020

Xethan commented Oct 2, 2020

Xethan commented Oct 2, 2020 • edited

Xethan commented Oct 2, 2020

NicolasHug commented Oct 2, 2020

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xethan Oct 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Oct 5, 2020 via email

Xethan commented Oct 6, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xethan commented Oct 2, 2020 •

edited

Xethan Oct 2, 2020 •

edited