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

docs: Remove tempfiles from docs #1193

Merged
merged 14 commits into from
Jan 22, 2025
Merged

docs: Remove tempfiles from docs #1193

merged 14 commits into from
Jan 22, 2025

Conversation

MarieS-WiMLDS
Copy link
Contributor

fixes #1178

@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as draft January 21, 2025 15:48
Copy link
Contributor

github-actions bot commented Jan 21, 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.py24196%86
   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.py19286%9–10
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.py50295%13, 172
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.py120099%215–>221, 223–>225
   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%187
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.py300100% 
   _show_versions.py310100% 
TOTAL276818392% 

Tests Skipped Failures Errors Time
612 3 💤 0 ❌ 0 🔥 2m 52s ⏱️

@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as ready for review January 21, 2025 15:59
@MarieS-WiMLDS MarieS-WiMLDS changed the title remove tempfiles from docs docs: Remove tempfiles from docs Jan 21, 2025
@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as draft January 21, 2025 16:04
@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as ready for review January 21, 2025 16:05
rouk1
rouk1 previously requested changes Jan 21, 2025
skore/src/skore/project/project.py Outdated Show resolved Hide resolved
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.

I don't love this solution because it makes all the examples dependent on one-another. If example 1 fails halfway through, then clear won't be run and example 2 will start with a non-empty project, which could make it fail, etc.

@auguste-probabl
Copy link
Contributor

But the clear method is fine IMO

@MarieS-WiMLDS
Copy link
Contributor Author

I don't love this solution because it makes all the examples dependent on one-another. If example 1 fails halfway through, then clear won't be run and example 2 will start with a non-empty project, which could make it fail, etc.

Is this likely to happen? Or it will trigger a red CI and therefore shouldn't land in main? (unless we force it)

@auguste-probabl
Copy link
Contributor

I don't love this solution because it makes all the examples dependent on one-another. If example 1 fails halfway through, then clear won't be run and example 2 will start with a non-empty project, which could make it fail, etc.

Is this likely to happen? Or it will trigger a red CI and therefore shouldn't land in main? (unless we force it)

Yes, tests fail all the time, and we would prefer that 1 example failed rather than all of them; that way it's easier to understand what went wrong and fix things

Another thing I thought of later is that this problem prevents us from running the examples in parallel

@MarieS-WiMLDS
Copy link
Contributor Author

I can't find any alternative solution for now to answer to all our goals. I created an issue to formalize this: #1200. In the mean time, if it's ok for you, I'd like to proceed with this solution to remove tempfile for this afternoon.

Copy link
Contributor

github-actions bot commented Jan 22, 2025

Documentation preview @ dc28842

@auguste-probabl
Copy link
Contributor

If example 1 fails halfway through, then clear won't be run and example 2 will start with a non-empty project, which could make it fail, etc.

To mitigate this could you follow https://sphinx-gallery.github.io/stable/configuration.html#abort-build-on-first-fail? This way if an example fails, we can see quicker where the problem comes from (rather than 10 other examples failing just because the first one failed)

Copy link
Contributor

@sylvaincom sylvaincom left a comment

Choose a reason for hiding this comment

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

Could we also apply this to the quick start example? 🙏

@sylvaincom sylvaincom self-requested a review January 22, 2025 14:41
Copy link
Contributor

@sylvaincom sylvaincom left a comment

Choose a reason for hiding this comment

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

Many thanks!

@sylvaincom sylvaincom self-requested a review January 22, 2025 14:42
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.

Nice!

@MarieS-WiMLDS MarieS-WiMLDS dismissed rouk1’s stale review January 22, 2025 14:51

2 approvals already.

@MarieS-WiMLDS MarieS-WiMLDS merged commit f7892b9 into main Jan 22, 2025
18 checks passed
@MarieS-WiMLDS MarieS-WiMLDS deleted the is/1178 branch January 22, 2025 14:52
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.

docs: In Sphinx examples, remove the temp files
5 participants