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: Use friendly verbose and colorish #962

Merged
merged 12 commits into from
Jan 6, 2025

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Dec 15, 2024

closes #959
closes #812

This PR proposes to:

  • use a default logger that does not show any information by default
  • introduce a --verbose option for the CLI or verbose parameter for the functions
  • a decorator that register a rich handler depending of the verbose parameter
  • introduce a rich console with cyan/orange colors (as the default skore theme) for showing the pertinent information.

TODO:

  • add a test to check if logger_context is behaving as expected.
  • add tests at the CLI level commands.

@glemaitre glemaitre marked this pull request as draft December 16, 2024 08:28
@glemaitre
Copy link
Member Author

glemaitre commented Dec 16, 2024

It would currently look like.

Terminal

image

Notebook

image

VS code

image

Documentation website

image

@glemaitre
Copy link
Member Author

So I switch from a decorator that was implicitly consuming verbose to a context manager that explicitly consume the verbose parameter. The former has a bug and was too much magical at my test. The context manager is much nicer in this regard.

@glemaitre glemaitre marked this pull request as ready for review December 20, 2024 10:29
Copy link
Member Author

@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.

OK, so it should be ready for review.

@MarieS-WiMLDS saw that we could refactor the argument in the CLI and use parent_parser. I don't add it in this PR to have the minimum diff, but it could be worth to do so for maintainability.

Another limitation of argparse here: I cannot pass verbose at the top parser and the subparser to allow skore --verbose launch ... and skore launch --verbose. This is something that click can nicely deal with and it is a pattern allowed in term of UX that I saw in other package (e.g. pixi).

Finally, @MarieS-WiMLDS spotted something weird in the quickstart where we can provide a working_dir for the create part but we don't do any cd for this directory in the launch. So it should fail. I'll try to craft a minimal non-regression test and it can be handled in another PR.

@glemaitre
Copy link
Member Author

@thomass-dev This last run will be fun for you :). In addition of the issue with the coverage (I maybe need to merge main), I segfault in Windows ;)

@thomass-dev thomass-dev force-pushed the main branch 2 times, most recently from beb4b05 to 469d1e5 Compare December 27, 2024 09:20
@sylvaincom
Copy link
Contributor

@thomass-dev I leave to you to merge this? 🙏

@thomass-dev thomass-dev force-pushed the main branch 3 times, most recently from ef3e8da to 469d1e5 Compare December 30, 2024 23:10
@thomass-dev thomass-dev force-pushed the main branch 2 times, most recently from ad7922d to 469d1e5 Compare December 31, 2024 14:42
@thomass-dev thomass-dev merged commit bc39557 into probabl-ai:main Jan 6, 2025
19 checks passed
thomass-dev pushed a commit that referenced this pull request Jan 10, 2025
closes #834

Investigate an API for a `EstimatorReport`.

#### TODO

- [x] Metrics
  - [x] handle string metrics has specified in the accessor
  - [x] handle callable metrics
  - [x] handle scikit-learn scorers
  - [x] use efficiently the cache as much as possible
  - [x] add testing for all of those features
- [x] allow to pass new validation set to functions instead of using the
internal validation set
  - [x] add a proper help and rich `__repr__`
- [x] Plots
  - [x] add the roc curve display
  - [x] add the precision recall curve display
  - [x] add prediction error display for regressor
  - [x] make proper testing for those displays
  - [x] add a proper `__repr__` for those displays
- [x] Documentation 
- [x] (done for the checked part) add an example to showcase all the
different features
- [x] find a way to show the accessors documentation in the page of
`EstimatorReport`. It could be a bit tricky because they are only
defined once the instance created.
- We need to have a look at the `series.rst` page from pandas to see how
they document this sort of pattern.
- [x] check the autocompletion: when typing `report.metrics.->tab` it
should provide the autocompetion. **edit**: having a stub file is
actually working. I prefer this than type hints directly in the file.
- Open questions
  - [x] we use hashing to retrieve external set.
- use the caching for the external validation set? To make it work we
need to compute the hash of potentially big arrays. This might more
costly than making the model predict.

#### Notes

This PR build upon:
- #962 to reuse the
`skore.console`
- #998 to be able to detect
clusterer in a consistent manner.
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.

Use printing instead of logging in non-verbose mode in CLI FEAT: output of the creation of a skore project
4 participants