FIX handle empty steps in `Pipeline` by gdacciaro · Pull Request #30203 · scikit-learn/scikit-learn

gdacciaro · 2024-11-03T23:08:59Z

Reference Issues/PRs
Fixes #30197
This pull request addresses the issue reported in scikit-learn issue #30197, where an IndexError occurs when attempting to access the last estimator in an empty Pipeline.
The current implementation raises an "out of range" error when the steps attribute is empty, which does not provide sufficient context for users.

Changes

Modified sklearn/pipeline.py:
- Updated _estimator_type method to return None if the pipeline is empty.
- Updated sklearn_tags method to return tags if the pipeline is empty.
Added tests in sklearn/tests/test_pipeline.py:
- Test for _estimator_type with classifier and regressor.
- Test for _estimator_type when the last step is not an estimator.
- Test for _estimator_type in an empty pipeline.
- Test for sklearn_tags with an empty pipeline.

…operty

github-actions · 2024-11-03T23:10:17Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 67bd522. Link to the linter CI: here}

adrinjalali

we should also add a test to make sure repr and html repr of the empty pipeline don't raise.

sklearn/tests/test_pipeline.py

doc/whats_new/upcoming_changes/sklearn.pipeline/30203.fix.rst

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

…replace direct _estimator_type checks with is_classifier() and is_regressor() assertions.

sklearn/tests/test_pipeline.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

gdacciaro · 2024-11-06T15:27:06Z

Hey @adrinjalali, the PR is completed on my end. Please let me know if any further reviews are needed. Thank you!

adrinjalali

LGTM. cc @glemaitre for the second review.

glemaitre

So here a couple comments just to simplify a bit the tests but otherwise it looks good.

doc/whats_new/upcoming_changes/sklearn.pipeline/30203.fix.rst

sklearn/tests/test_pipeline.py

sklearn/utils/tests/test_estimator_html_repr.py

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

gdacciaro · 2024-11-07T18:40:54Z

I have committed the suggestions, thank you very much!
@glemaitre

glemaitre

I just pushed a small fix to make the linter happy by removing a trailing space.

glemaitre · 2024-11-08T09:36:27Z

Enabling auto-merge

glemaitre · 2024-11-08T10:24:56Z

adding the label "To Backport" and the milestone to not forget this PR.

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

glemaitre · 2024-11-08T10:48:13Z

Backported in 1.6.X

Fix: Raise informative error for empty Pipeline in _estimator_type pr…

7d47495

…operty

github-actions bot added the module:pipeline label Nov 3, 2024

gdacciaro added 2 commits November 4, 2024 17:38

fix rendering html empty pipeline

793d3e3

Merge branch 'main' into fix/empty-pipeline-informative-error

5003cf7

gdacciaro changed the title ~~ENH - add informative error for empty Pipeline in _estimator_type property~~ FIX - rendering html from an empty pipeline Nov 4, 2024

formatted test and added the news fragment

0703a3e

adrinjalali reviewed Nov 5, 2024

View reviewed changes

sklearn/tests/test_pipeline.py Outdated Show resolved Hide resolved

doc/whats_new/upcoming_changes/sklearn.pipeline/30203.fix.rst Outdated Show resolved Hide resolved

gdacciaro and others added 4 commits November 5, 2024 14:48

Update doc/whats_new/upcoming_changes/sklearn.pipeline/30203.fix.rst

894c11f

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Merge branch 'main' into fix/empty-pipeline-informative-error

905901c

Merge branch 'main' into fix/empty-pipeline-informative-error

0d1324a

Add smoke tests for repr and HTML representation of empty pipelines; …

1ca2eff

…replace direct _estimator_type checks with is_classifier() and is_regressor() assertions.

adrinjalali reviewed Nov 6, 2024

View reviewed changes

sklearn/tests/test_pipeline.py Outdated Show resolved Hide resolved

sklearn/tests/test_pipeline.py Outdated Show resolved Hide resolved

gdacciaro and others added 3 commits November 6, 2024 09:09

Update sklearn/tests/test_pipeline.py

67cea19

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Update sklearn/tests/test_pipeline.py

5f4f759

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

fix: fixed test for pipeline with non-estimator as the last step"

2d23f38

Merge branch 'main' into fix/empty-pipeline-informative-error

e85b0c5

adrinjalali approved these changes Nov 7, 2024

View reviewed changes

glemaitre self-requested a review November 7, 2024 16:59

glemaitre reviewed Nov 7, 2024

View reviewed changes

gdacciaro and others added 6 commits November 7, 2024 19:32

Update doc/whats_new/upcoming_changes/sklearn.pipeline/30203.fix.rst

5abacef

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Update sklearn/tests/test_pipeline.py

d902830

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Update sklearn/tests/test_pipeline.py

777f5b5

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Update sklearn/utils/tests/test_estimator_html_repr.py

e82a0f7

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Update sklearn/utils/tests/test_estimator_html_repr.py

bcf012e

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Merge branch 'main' into fix/empty-pipeline-informative-error

8265ff1

glemaitre self-requested a review November 8, 2024 09:33

linter

67bd522

glemaitre approved these changes Nov 8, 2024

View reviewed changes

glemaitre changed the title ~~FIX - rendering html from an empty pipeline~~ FIX handle empty steps in Pipeline Nov 8, 2024

glemaitre enabled auto-merge (squash) November 8, 2024 09:36

glemaitre added this to the 1.6 milestone Nov 8, 2024

glemaitre added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Nov 8, 2024

glemaitre merged commit 6ce072a into scikit-learn:main Nov 8, 2024

glemaitre added a commit that referenced this pull request Nov 8, 2024

FIX handle empty steps in Pipeline (#30203)

66b71f0

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

gdacciaro deleted the fix/empty-pipeline-informative-error branch November 8, 2024 13:51

Uh oh!

Conversation

gdacciaro commented Nov 3, 2024 • edited by adrinjalali Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gdacciaro commented Nov 6, 2024

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gdacciaro commented Nov 7, 2024

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Nov 8, 2024

Uh oh!

glemaitre commented Nov 8, 2024

Uh oh!

glemaitre commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gdacciaro commented Nov 3, 2024 •

edited by adrinjalali

Loading

github-actions bot commented Nov 3, 2024 •

edited

Loading