Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: Test with the 3 latest major versions of scikit-learn #916

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

thomass-dev
Copy link
Collaborator

@thomass-dev thomass-dev commented Dec 11, 2024

Refactor skore.yml workflow:

  • launch tests with the 3 latest major versions of scikit-learn on os: "ubuntu-latest" and python: "3.12",
  • speed-up tests by installing dependencies in venvand caching it.

And make _add_scorers compatible with sklearn ==1.4.

Copy link
Contributor

@augustebaum augustebaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure skore is critical to the point of making our CI run 4 times slower

@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch 3 times, most recently from 11f3c35 to 1e08c6e Compare December 11, 2024 13:56
Copy link
Contributor

github-actions bot commented Dec 11, 2024

Coverage

pytest coverage report
FileStmtsMissCoverMissing
src/skore
   __init__.py120100% 
   __main__.py811 80%
   exceptions.py30100% 
src/skore/cli
   __init__.py50100% 
   cli.py330100% 
   color_format.py4332 90%
   launch_dashboard.py26150 39%
   quickstart_command.py140100% 
src/skore/item
   __init__.py210100% 
   cross_validation_item.py137102 93%
   item.py41130 68%
   item_repository.py4221 93%
   media_item.py7041 94%
   numpy_array_item.py2511 93%
   pandas_dataframe_item.py3411 95%
   pandas_series_item.py3411 95%
   polars_dataframe_item.py3211 94%
   polars_series_item.py2711 94%
   primitive_item.py2721 92%
   sklearn_base_estimator_item.py3311 95%
   skrub_table_report_item.py1011 86%
src/skore/persistence
   __init__.py00100% 
   abstract_storage.py2210 95%
   disk_cache_storage.py3311 95%
   in_memory_storage.py200100% 
src/skore/project
   __init__.py30100% 
   create.py5280 88%
   load.py2330 89%
   open.py140100% 
   project.py6444 91%
src/skore/sklearn
   __init__.py30100% 
   find_ml_task.py3532 89%
   types.py20100% 
src/skore/sklearn/cross_validation
   __init__.py20100% 
   cross_validation_helpers.py4741 90%
   cross_validation_reporter.py3511 95%
src/skore/sklearn/cross_validation/plots
   __init__.py00100% 
   compare_scores_plot.py2912 92%
   timing_plot.py2911 94%
src/skore/sklearn/train_test_split
   __init__.py00100% 
   train_test_split.py3421 94%
src/skore/sklearn/train_test_split/warning
   __init__.py80100% 
   high_class_imbalance_too_few_examples_warning.py1732 78%
   high_class_imbalance_warning.py1821 88%
   random_state_unset_warning.py1111 87%
   shuffle_true_warning.py901 91%
   stratify_is_set_warning.py1111 87%
   time_based_column_warning.py2212 89%
   train_test_split_warning.py510 80%
src/skore/ui
   __init__.py00100% 
   app.py2552 71%
   dependencies.py710 86%
   project_routes.py500100% 
src/skore/utils
   __init__.py00100% 
   _logger.py2140 84%
   _show_versions.py310100% 
src/skore/view
   __init__.py00100% 
   view.py50100% 
   view_repository.py1621 83%
TOTAL138010791% 

Tests Skipped Failures Errors Time
259 0 💤 0 ❌ 0 🔥 34.149s ⏱️

@thomass-dev
Copy link
Collaborator Author

thomass-dev commented Dec 11, 2024

Not sure skore is critical to the point of making our CI run 4 times slower

Older versions of scikit-learn are only tested on ubuntu-latest python 3.12.

@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch 21 times, most recently from 39009b6 to f6b2d7b Compare December 13, 2024 19:46
@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch 12 times, most recently from c7f5abd to 7bb938b Compare January 8, 2025 19:24
@thomass-dev thomass-dev requested a review from glemaitre January 8, 2025 19:33
@thomass-dev thomass-dev marked this pull request as ready for review January 8, 2025 19:38
@thomass-dev thomass-dev requested a review from rouk1 January 8, 2025 19:43
@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch from 7bb938b to 33e1951 Compare January 8, 2025 19:43
@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch from 33e1951 to 9ce6e5b Compare January 8, 2025 19:52
@glemaitre
Copy link
Member

So as a workaround for the older scikit-learn function, I think that you can:

  • add a parameter estimator to _add_scorers
  • pass self.estimator to the call to _add_scorers in the CrossValidationReporter.

Basically, in the case that scoring is None, we would default to self.estimator.score. However, in practice it will never happen because we are creating a list of scorer to scoring is never None. So it just a workaround.

I think it is worth a small comment to know why we pass the estimator around and actually don't expect to use it.

@glemaitre
Copy link
Member

I open #1062 that should solve the issue.

Addressing the issue specified in
#916 (comment)

Because in scikit-learn 1.4, `check_estimator` does not accept
`estimator=None`. Luckily, we have the available estimator so we can
provide it to `_add_scorers` as a workaround and since `scoring is not
None` we don't have any change of behaviour anyway.
@thomass-dev
Copy link
Collaborator Author

Thanks @glemaitre for your fix, all good. It's ready to merge @rouk1 .

# Conflicts:
#	skore/src/skore/sklearn/cross_validation/cross_validation_helpers.py
rouk1
rouk1 previously approved these changes Jan 9, 2025
Copy link
Contributor

@rouk1 rouk1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, the planet says thx for caching venvs 💚

@thomass-dev thomass-dev force-pushed the ci-scikit-learn-matrix branch from 63fd0f0 to 8a08182 Compare January 9, 2025 10:20
@thomass-dev thomass-dev merged commit 24a7c1b into main Jan 9, 2025
21 checks passed
@thomass-dev thomass-dev deleted the ci-scikit-learn-matrix branch January 9, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants