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

feat: Modify the user API of skore to respect "what you put is what you get" principle #1052

Merged
merged 45 commits into from
Jan 20, 2025

Conversation

thomass-dev
Copy link
Collaborator

@thomass-dev thomass-dev commented Jan 7, 2025

Closes #1045
Closes #734

Refactor the user API to hide all notions of Item, View, and to respect "what you put is what you get" from a user's point of view.
Among others:

  • Hide item classes in sub-directory to be less visible by users
  • All @cached_property in items have been removed, because items are not used anymore directly by users
  • Remove the ability to store anything other than strings in MediaItem
  • Explode MediaItem in new item classes
  • Add PickleItem class which can persist any object when it cannot be otherwise
  • Add display_as parameter to project.put to control how a string is displayed in the frontend
  • Remove project.put_item in such way user need only to use project.put
  • The project.get function always returns what the user has put
  • The project.get and project.get_item_versions have been merged
  • The CrossValidationItem has been replaced by a CrossValidationReporterItem based on pickle
    • To go fast, and because a report is composed of complex objects, such as estimator, X and y, i've made the choice to persist the report as a pickle. That way, we can get a report from the persistency without effort. In a next iteration, we should think about how to persist more efficiencly and env-independently a report which can be rebuilt entirely from the persistency.

  • hide item API
  • update each item classes to return their original objects
    • cross validation reporter
    • pillow image
    • plotly figure
    •  altair figure
    • matplotlib figure
    • media item, to only accept str with display_as
    • primitive -> already 🆗
    • pandas dataframe -> already 🆗
    • polars dataframe -> already 🆗
    • pandas series -> already 🆗
    • polars series -> already 🆗
    • numpy array -> already 🆗
    •  scikit-learn estimator -> already 🆗
  • set note in each factory
  • update put to allow the new parameter display_as
  • hide view API
  • move repr_html to reporters -> feat: Move (or remove) the repr feature of Item class #1161
  • change the way numpy array are serialized -> postponed feat: Change the way NumPy array are persisted #1159

@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from afd8291 to 666f094 Compare January 7, 2025 14:52
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from 06ff76f to 0277edb Compare January 7, 2025 15:46
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from c1b9341 to f4552e9 Compare January 7, 2025 15:56
@tuscland tuscland added this to the Skore 0.7 milestone Jan 7, 2025
@glemaitre glemaitre requested review from glemaitre and removed request for glemaitre January 8, 2025 09:59
@glemaitre
Copy link
Member

glemaitre commented Jan 8, 2025

I think that this PR will also close:

@thomass-dev I let you amend your summary if it is the case.

Co-authored-by: Auguste Baum <[email protected]>

commit 86281ca
Author: Auguste Baum <[email protected]>
Date:   Wed Jan 8 12:00:46 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/tests/integration/ui/test_ui.py

commit 9f6f193
Author: Thomas S <[email protected]>
Date:   Wed Jan 8 11:52:12 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/tests/integration/ui/test_ui.py

commit cd385f6
Author: Auguste Baum <[email protected]>
Date:   Wed Jan 8 11:41:10 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/src/skore/persistence/item/sklearn_base_estimator_item.py

commit 41c5355
Author: Thomas S <[email protected]>
Date:   Wed Jan 8 11:27:16 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/tests/integration/sklearn/test_cross_validate.py

commit a7d956e
Author: Auguste Baum <[email protected]>
Date:   Wed Jan 8 11:14:59 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/tests/unit/view/test_view_repository.py

commit 38446cf
Author: Thomas S <[email protected]>
Date:   Wed Jan 8 11:01:43 2025 +0100

    mob next [ci-skip] [ci skip] [skip ci]

    lastFile:skore/src/skore/persistence/item/pickle_item.py

commit a6e2a79
Author: Thomas S <[email protected]>
Date:   Wed Jan 8 10:51:07 2025 +0100

    mob start [ci-skip] [ci skip] [skip ci]

Co-authored-by: Auguste Baum <[email protected]>
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from 83747f7 to aff2a5f Compare January 8, 2025 11:25
@thomass-dev thomass-dev self-assigned this Jan 10, 2025
# Conflicts:
#	skore/src/skore/project/create.py
#	skore/tests/unit/project/test_project.py
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from a02785f to 862dc49 Compare January 10, 2025 09:22
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from e3d0aa8 to 80df8db Compare January 10, 2025 14:15
Copy link
Contributor

github-actions bot commented Jan 10, 2025

Coverage

Coverage Report for backend
FileStmtsMissCoverMissing
venv/lib/python3.12/site-packages/skore
   __init__.py140100% 
   __main__.py8180%19
   exceptions.py30100% 
venv/lib/python3.12/site-packages/skore/cli
   __init__.py50100% 
   cli.py33385%104, 111, 117
   color_format.py43390%35–>40, 41–43
   launch_dashboard.py261539%36–57
   quickstart_command.py14750%37–51
venv/lib/python3.12/site-packages/skore/persistence
   __init__.py00100% 
venv/lib/python3.12/site-packages/skore/persistence/item
   __init__.py59491%89, 100–103
   altair_chart_item.py24193%14
   cross_validation_reporter_item.py1051288%28–41, 125–126, 254, 257
   item.py411368%90, 93, 97–117
   matplotlib_figure_item.py30194%18
   media_item.py240100% 
   numpy_array_item.py22192%14
   pandas_dataframe_item.py31194%14
   pandas_series_item.py31194%14
   pickle_item.py290100% 
   pillow_image_item.py29194%14
   plotly_figure_item.py25193%14
   polars_dataframe_item.py29194%14
   polars_series_item.py24193%14
   primitive_item.py25291%13–15
   sklearn_base_estimator_item.py28194%14
   skrub_table_report_item.py10186%11
venv/lib/python3.12/site-packages/skore/persistence/repository
   __init__.py30100% 
   item_repository.py59591%15–16, 202–203, 226
   view_repository.py19381%9–10, 76
venv/lib/python3.12/site-packages/skore/persistence/storage
   __init__.py40100% 
   abstract_storage.py220100% 
   disk_cache_storage.py33195%44
   in_memory_storage.py200100% 
venv/lib/python3.12/site-packages/skore/persistence/view
   __init__.py00100% 
   view.py50100% 
venv/lib/python3.12/site-packages/skore/project
   __init__.py30100% 
   create.py52888%116–122, 132–133, 140–141
   load.py22388%45–47
   open.py140100% 
   project.py39294%12, 159
venv/lib/python3.12/site-packages/skore/sklearn
   __init__.py50100% 
   _base.py140497%91, 94, 168–>173, 183–184
   find_ml_task.py45195%71–>87, 80–>87, 86
   types.py20100% 
venv/lib/python3.12/site-packages/skore/sklearn/_cross_validation
   __init__.py60100% 
   metrics_accessor.py1630100% 
   report.py870100% 
venv/lib/python3.12/site-packages/skore/sklearn/_estimator
   __init__.py60100% 
   metrics_accessor.py265497%149–158, 183–>236, 191, 434–>437, 783–>786
   report.py121099%216–>222, 224–>226
   utils.py11110%1–19
venv/lib/python3.12/site-packages/skore/sklearn/_plot
   __init__.py40100% 
   precision_recall_curve.py121198%229–>246, 317
   prediction_error.py970100% 
   roc_curve.py1280100% 
   utils.py880100% 
venv/lib/python3.12/site-packages/skore/sklearn/cross_validation
   __init__.py20100% 
   cross_validation_helpers.py47490%104–>136, 123–126
   cross_validation_reporter.py35195%177
venv/lib/python3.12/site-packages/skore/sklearn/cross_validation/plots
   __init__.py00100% 
   compare_scores_plot.py292416%10, 28–116
   timing_plot.py292417%10, 26–123
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split
   __init__.py00100% 
   train_test_split.py36294%16–17
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split/warning
   __init__.py80100% 
   high_class_imbalance_too_few_examples_warning.py17378%16–18, 80
   high_class_imbalance_warning.py18288%16–18
   random_state_unset_warning.py11187%15
   shuffle_true_warning.py9091%44–>exit
   stratify_is_set_warning.py11187%15
   time_based_column_warning.py22189%17, 69–>exit
   train_test_split_warning.py5180%21
venv/lib/python3.12/site-packages/skore/ui
   __init__.py00100% 
   app.py32868%26, 55–60, 72–74
   dependencies.py7186%12
   project_routes.py610100% 
venv/lib/python3.12/site-packages/skore/utils
   __init__.py00100% 
   _accessor.py70100% 
   _logger.py21484%14–18
   _patch.py11546%19–35
   _progress_bar.py280100% 
   _show_versions.py310100% 
TOTAL277319692% 

Tests Skipped Failures Errors Time
609 3 💤 0 ❌ 0 🔥 3m 24s ⏱️

@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from c0ad30c to c8f3ecf Compare January 13, 2025 14:30
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch 2 times, most recently from 510bfa1 to 6255ada Compare January 13, 2025 17:01
MarieS-WiMLDS

This comment was marked as resolved.

MarieS-WiMLDS
MarieS-WiMLDS previously approved these changes Jan 17, 2025
Copy link
Contributor

@MarieS-WiMLDS MarieS-WiMLDS left a comment

Choose a reason for hiding this comment

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

I'm really happy with what I tested so far :D.
I have to go, I'll do further tests on monday, but so far so good!!

I have 2 questions, not blocking:

  • did you think of a way to fetch a version "in the middle"? The lastest parameter in get is a bit weak imo, maybe version, like in set_note, could be better.
  • why did you choose to ask for capital strings in display_as? it feels a bit weird to me, i'd prefer lower case.

image

@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from 7ff8318 to 1022e7a Compare January 17, 2025 19:53
@thomass-dev thomass-dev force-pushed the what-you-put-is-what-you-get branch from 6116205 to 9fe0601 Compare January 17, 2025 20:16
@thomass-dev
Copy link
Collaborator Author

@glemaitre

@thomass-dev I don't see put and get in the api.rst. Maybe we could add these two functions there?

Fixed 087fcaf .

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.

Awesome work!

Copy link
Contributor

@auguste-probabl auguste-probabl left a comment

Choose a reason for hiding this comment

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

The Project docstring needs to be updated

glemaitre
glemaitre previously approved these changes Jan 20, 2025
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Approved upon the point that we discussed earlier.

@thomass-dev
Copy link
Collaborator Author

The Project docstring needs to be updated

Fixed in e465440.

Copy link
Contributor

@MarieS-WiMLDS MarieS-WiMLDS left a comment

Choose a reason for hiding this comment

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

youhou!

@thomass-dev thomass-dev merged commit 5cb9468 into main Jan 20, 2025
13 of 18 checks passed
@thomass-dev thomass-dev deleted the what-you-put-is-what-you-get branch January 20, 2025 13:59
thomass-dev added a commit that referenced this pull request Jan 20, 2025
Following #1052 , change the API to be compliant with `set_note` and
@MarieS-WiMLDS comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants