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

chore: Hide view functions from project #1013

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarieS-WiMLDS
Copy link
Contributor

@MarieS-WiMLDS MarieS-WiMLDS commented Dec 23, 2024

Will actually be closed by #1052

We have functions about views in project which are documented & public, while we offer no specific feature related to a view. So far, it's something we want to deal with internally.
Suggesting this solution, but not sure it's the best since the view functions still appear in the autocomplete. Also, there is still the "view_repository" as one of the parameters of Project while it would be best to be hidden.

Capture d’écran du 2024-12-23 17-35-26

@MarieS-WiMLDS MarieS-WiMLDS changed the title feat: Hide view functions from project chore: Hide view functions from project Dec 23, 2024
Copy link
Contributor

github-actions bot commented Dec 23, 2024

Coverage

pytest coverage report
FileStmtsMissCoverMissing
src/skore
   __init__.py180100% 
   __main__.py811 80%
   exceptions.py40100% 
src/skore/cli
   __init__.py80100% 
   cli.py320100% 
   launch_dashboard.py22120 42%
   quickstart_command.py1220 83%
src/skore/item
   __init__.py210100% 
   cross_validation_item.py127102 92%
   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__.py40100% 
   create.py5080 88%
   load.py2330 89%
   project.py6244 91%
src/skore/sklearn
   __init__.py30100% 
   find_ml_task.py1923 85%
   types.py20100% 
src/skore/sklearn/cross_validation
   __init__.py20100% 
   cross_validation_helpers.py4141 89%
   cross_validation_reporter.py3711 95%
src/skore/sklearn/cross_validation/plots
   __init__.py00100% 
   compare_scores_plot.py2912 92%
   timing_normalized_plot.py2911 94%
   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% 
   _show_versions.py290100% 
src/skore/view
   __init__.py00100% 
   view.py50100% 
   view_repository.py1621 83%
TOTAL12999991% 

Tests Skipped Failures Errors Time
194 0 💤 0 ❌ 0 🔥 46.174s ⏱️

@MarieS-WiMLDS MarieS-WiMLDS linked an issue Dec 24, 2024 that may be closed by this pull request
@thomass-dev
Copy link
Collaborator

We have functions about views in project which are documented & public, while we offer no specific feature related to a view. So far, it's something we want to deal with internally.

Managing a view from the developer interface was in the original design. We don't want to do this anymore? (ping @tuscland )

@tuscland
Copy link
Member

I agree that it is weird to have those repositories exposed. This is private stuff.

However, being able to build views programmatically can be useful. I would like to discuss with Marie offline about her intent in creating this issue. Maybe it is part of a larger project that I am not aware of at the moment.

@thomass-dev thomass-dev marked this pull request as draft December 26, 2024 13:01
@thomass-dev
Copy link
Collaborator

I've re-drafted the PR as long as there is an open discussion.

@thomass-dev thomass-dev force-pushed the main branch 7 times, most recently from ad7922d to 469d1e5 Compare December 31, 2024 14:42
@MarieS-WiMLDS
Copy link
Contributor Author

My intent is to have a library that is as simple as possible for the user. When they see a function, it really provides value to them, it helps them for a given use case they have appart from skore.
It will be useful to create repositories programmatically when the user can do something with their views. Today, it helps to visualise, but that's it, there is no sharing nor export. Therefore it's useful only for them: it's not something that will be done in high amounts, hence it seems to me that there is no need to create it automatically.

@tuscland
Copy link
Member

tuscland commented Jan 7, 2025

@thomass-dev as discussed with Marie, it is a good decision to reduce the public API space and relegate View to private scope.

@tuscland tuscland added this to the Skore 0.7 milestone Jan 7, 2025
@glemaitre
Copy link
Member

So if I follow well enough, this PR is superseded by #1052 and do not need any review and will be closed by #1052?

@MarieS-WiMLDS
Copy link
Contributor Author

MarieS-WiMLDS commented Jan 8, 2025

No because #1052 is about the items, while this is about the views.
However it is the same purpose behind indeed, we want to simplify the API and only ask the user to master new vocab only if it brings real value to them.

@glemaitre
Copy link
Member

OK I see, I got confused because of the related issue (#1045 (comment)) where it is also mentioned that views should become private.

It would really help to have a proper description in PR summary.

@MarieS-WiMLDS
Copy link
Contributor Author

ah, indeed, the TODO was enriched!
Ok so you're right, it will be tackled in #1052!
I'd rather keep this PR open until it's really done though.

@glemaitre
Copy link
Member

Yes, it is just that we can add closes #... in the summary of the PR to close it at merge.

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.

DOC Incomplete API pages
4 participants