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

fix: Fix quickstart instruction in CLI #1048

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

thomass-dev
Copy link
Collaborator

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

Closes #995 .

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Coverage

pytest coverage report
FileStmtsMissCoverMissing
src/skore
   __init__.py120100% 
   __main__.py811 80%
   exceptions.py40100% 
src/skore/cli
   __init__.py50100% 
   cli.py350100% 
   launch_dashboard.py26150 39%
   quickstart_command.py1520 87%
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.py5480 88%
   load.py2330 89%
   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.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% 
   _logger.py2140 84%
   _show_versions.py290100% 
src/skore/view
   __init__.py00100% 
   view.py50100% 
   view_repository.py1621 83%
TOTAL134310791% 

Tests Skipped Failures Errors Time
210 0 💤 0 ❌ 0 🔥 48.183s ⏱️

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 tried the fix and it works for me, thanks!
However I still don't understand why the test was crashing, did you understand? I have some doubts that it's doing its job since it didn't catch the error before.

@augustebaum
Copy link
Contributor

I tried the fix and it works for me, thanks! However I still don't understand why the test was crashing, did you understand? I have some doubts that it's doing its job since it didn't catch the error before.

I'm guessing it's because the test is very basic and doesn't actually run the real launch command.

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.

Nice, thanks!

@augustebaum augustebaum merged commit a4695e0 into main Jan 6, 2025
19 checks passed
@augustebaum augustebaum deleted the fix-cli-quickstart branch January 6, 2025 14:26
@thomass-dev
Copy link
Collaborator Author

thomass-dev commented Jan 6, 2025

@MarieS-WiMLDS the test was wrong and has replicated the same behavior of the bug, this is why it was green.

image

It tested that the directory was well passed to the create function, but not to the launch function.
If we had tested assert launch_project_name == "hello/my_project.skore", we would have discovered the bug.
Thanks for the feedback, well spotted.

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.

[cli] Quickstart working-directory option not working
3 participants