ENH Add themes for HTML display. Add dark theme #26862

9Y5 · 2023-07-19T19:57:09Z

Reference Issues/PRs

Implements feature request for styled diagrams. See #26364

Screen.Recording.2023-07-20.at.11.14.01.PM.mov

Updated the documentation on Displaying Pipelines:

What does this implement/fix? Explain your changes.

This MR adds an extensible way to generate themes for HTML renders.
A dark theme is added to show how it is used.

Without any theme given, it falls back to the default "light" theme:

When a theme such as themes.DARK is given, it can set the theme to dark theme:

Any other comments?

github-actions · 2023-07-19T19:58:39Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: bb1f1f6. Link to the linter CI: here}

thomasjpfan · 2023-07-19T20:24:35Z

Normally, one does not call estimator_html_repr directly, but the HTML repr is shown automatically when outputting an estimator. I think there needs to be an automatic way to have the HTML repr pick up the browser's or Jupyter's preference.

Normally this is done with a prefers-color-scheme CSS media query, but it is not supported in jupyterlab yet: jupyterlab/jupyterlab#8777 In the mean time, I would be okay with a workaround that does not involve javascript.

9Y5 · 2023-07-19T20:32:39Z

Normally, one does not call estimator_html_repr directly, but the HTML repr is shown automatically when outputting an estimator. I think there needs to be an automatic way to have the HTML repr pick up the browser's or Jupyter's preference.

Normally this is done with a prefers-color-scheme CSS media query, but it is not supported in jupyterlab yet: jupyterlab/jupyterlab#8777 In the mean time, I would be okay with a workaround that does not involve javascript.

Yes, agree this change won't solve the dark theme problem (since as you said, this would require some detection on the browser). Instead, it adds a theming layer to the HTML renders.
I think this change can still expose some low-level methods to change the theme if they need it for report generation purposes on Jupyter. wdyt?

Also later, we can add dark theme with detection using prefers-color-scheme more easily once support on jupyterlab is ready.

@thomasjpfan how do you think about the overall approach?

betatim · 2023-07-20T09:06:21Z

It seems the main use-case of the light/dark mode switching is to match it up to the light/dark mode used by the notebook that the HTML representation is rendered into. So I think we should add this if we have a way to query the notebook viewer (JupyterLab, VS Code, others?) to find out if they are using light/dark mode. This is another level up in complexity from using CSS prefers-color-scheme because it seems JupyterLab (and probably others) let the user select light/dark mode independent of their OS/browser preference. Does someone know if prefers-color-scheme only reflects the OS/browser level preference or if JupyterLab could set it as a result of the user toggling between light/dark mode?

Overall I think adding a way for the user to set the theme for the HTML repr by hand is not that attractive because it seems it would add yet another toggle (OS level, JupyterLab level, and scikit-learn level). And all of them are not connected to each other :-/

9Y5 · 2023-07-20T11:15:30Z

Ok, thinking about it, it's still best to provide a way to override the settings in anycase.

For example, I may have dark mode on, but I want my diagrams to appear light-themed. In that case, we would still need a way to let user to define the theme that they want.

Maybe something like this:

betatim · 2023-07-20T11:31:01Z

For me the question is "why would you want to do that?" - this is a serious question from me as someone who never uses dark mode. So I'm a weird person because a lot of people love dark mode and/or being able to switch. However it does mean I can't imagine why you'd ever want to have the notebook in dark (light) mode but the estimator reprs in light (dark) mode. Do you have an idea?

9Y5 · 2023-07-20T11:45:16Z

@betatim I don't use dark mode myself either, and not aiming to solve the dark-mode problem with this MR.

However, I do use Jupyter to generate reports and diagrams. And having the ability to style the pipeline diagram will certainly be useful.
Right now, we start with DARK and LIGHT themes, but could easily extend do Solarized, low-contrast etc.
I could add a few more themes to showcase this functionality.

9Y5 · 2023-07-20T12:13:04Z

Put differently, this MR:

Can resolve Adding dark mode for pipeline diagram #26364 and similar requests to style the pipeline diagram.
It is not feasible and complex to add "dark mode" detection today. We can tackle this in a separate larger MR. (If it's even still wanted)

adrinjalali · 2023-07-20T13:22:03Z

To me it seems like we need to wait for jupyterlab/jupyterlab#8777 to be able to have an automated themed' fix.

In the meantime, we could make set_config(display="diagram-dark") to explicitly set the theme to dark, otherwise use the default provided in CSS.

9Y5 · 2023-07-20T14:04:48Z

In the meantime, we could make set_config(display="diagram-dark") to explicitly set the theme to dark, otherwise use the default provided in CSS.

Interesting, yes set_config() may be a good place to set it than on Pipeline or make_pipeline.

Approach 1 - In Pipeline, add new optional attribute `theme`:

pipe = Pipeline(steps) # if `theme` not give, will fallback to current style
pipe = Pipeline(steps, theme=themes.DARK) # `theme` is set to dark style

Pros: Seems intuitive (subjective) to add here where I'm declaring the pipeline.
Cons: Could be argued to keep Pipeline focused on ML model params.

Approach 2 - In Config, modify existing optional attribute `display`:

set_config(display="diagram-dark") # current values are ("diagram", "text")

Pros: Keeps Pipeline focused on ML model, config for display-related configs.
Cons: Adding ("diagram-dark", "diagram", "text") now makes us stuck on just 2 types of styles? What if we want ("diagram-dark", "diagram-style-2", "diagram-style-3", "diagram", "text")

Approach 3 - In Config, add new optional attribute `theme`:

set_config(theme=themes.DARK) # set a new config value `theme` that defaults to `themes.LIGHT`

Pros: Keeps Pipeline focused on ML model, config for display-related configs; theme is separate from display mode.
Cons: Not as intuitive (subjective) to set the display of the pipeline.

I'm leaning towards Approach 1 or 3. But what do you think?

adrinjalali · 2023-07-20T15:13:51Z

I don't think we need to support multiple themes, and it certainly doesn't make sense to add this as an argument to pipeline.

And this is only a temporary solution, in the long run, we'd depend on whatever the css info tells us.

9Y5 · 2023-08-01T13:59:59Z

@thomasjpfan @adrinjalali pls help review again, thank you

Looks like it can close #26364

9Y5 · 2023-08-01T14:07:20Z

I was thinking, I can start a next PR (and a few more in sequence) to refactor this themes section of the codebase in small and safe steps. The goal being to bring better support for CSS variables.

At some point, it may be possible to set themes on jupyter-themes, that can propagate the styles to sklearn's diagrams.

Do let me know wdyt this.

thomasjpfan

Overall, I am happy with using css variables and prefers-color-scheme as the first step.

thomasjpfan · 2023-08-01T17:43:18Z

sklearn/utils/_estimator_html_repr.py

@@ -190,13 +190,31 @@ def _write_estimator_html(

 _STYLE = """
 #$id {
-  color: black;
+  --sklearn-color-1: black;


I know I suggested the template for these variable names, but do you see better names for these color options given their usage?

For example, --sklearn-color-1 can be renamed to --sklearn-font-color.

Hmm true, could be a matter of naming preference: whether we wanna define by color scheme (in which numbering 1 to 5 would be fine I think), or by its function/purpose. Maybe for now, we can do by purpose until we require theming it. Check out the latest commit

9Y5 · 2023-08-02T17:17:26Z

let me re-open, need to fix the branches

9Y5 · 2023-08-02T17:17:42Z

Reopening

9Y5 · 2023-08-05T07:30:58Z

@thomasjpfan @adrinjalali PR is ready for review again

thomasjpfan

LGTM

betatim

LGTM. I think this is a good improvement over what we have right now.

Add themes for HTML display. Add dark theme.

d0ce1f6

github-actions bot added the module:utils label Jul 19, 2023

9Y5 added 3 commits July 20, 2023 04:01

Fix lint.

5ee57a5

Remove redundant default theme.

838499c

Add tests and comments.

493be2a

betatim changed the title ~~[WIP] Add themes for HTML display. Add dark theme. See #26364~~ [WIP] Add themes for HTML display. Add dark theme Jul 20, 2023

9Y5 added 3 commits July 20, 2023 19:34

Add theme from pipeline.

a73f13d

Add documentaiton

0d7ca4f

Add for make_pipeline.

5e66b4e

9Y5 mentioned this pull request Jul 20, 2023

Adding dark mode for pipeline diagram #26364

Closed

9Y5 added 2 commits July 20, 2023 20:47

Fix unit test.

361e22f

Add more unit tests. Parameterize the template.

29e1aae

Add more unit tests.

e739ee2

9Y5 changed the title ~~[WIP] Add themes for HTML display. Add dark theme~~ [MRG] Add themes for HTML display. Add dark theme Jul 20, 2023

9Y5 and others added 2 commits July 20, 2023 22:14

Merge branch 'main' into add-themes

03a0fb0

Change to set_config to set theme.

dcd5a44

9Y5 added 3 commits July 20, 2023 23:45

Fix unit test.

09048b4

Update unit test.

148680f

Update whats new. Add docstrings.

32fe9b0

thomasjpfan reviewed Aug 1, 2023

View reviewed changes

9Y5 force-pushed the add-themes branch 2 times, most recently from 5953ca8 to f0c5238 Compare August 2, 2023 16:15

9Y5 requested a review from thomasjpfan August 2, 2023 16:19

9Y5 force-pushed the add-themes branch 3 times, most recently from adcc5e2 to 11516c2 Compare August 2, 2023 16:56

Use descriptive css names.

450bdec

9Y5 closed this Aug 2, 2023

9Y5 force-pushed the add-themes branch fro 9E88 m 11516c2 to 9c96671 Compare August 2, 2023 17:07

Merge branch 'main' into add-themes2

7e96b08

9Y5 deleted the add-themes branch August 2, 2023 17:20

9Y5 restored the add-themes branch August 2, 2023 17:20

9Y5 reopened this Aug 2, 2023

9Y5 added 2 commits August 3, 2023 08:37

Merge branch 'main' into add-themes

ce769f7

Merge branch 'main' into add-themes

9b1aa0a

thomasjpfan changed the title ~~[MRG] Add themes for HTML display. Add dark theme~~ ENH Add themes for HTML display. Add dark theme Aug 7, 2023

thomasjpfan approved these changes Aug 7, 2023

View reviewed changes

thomasjpfan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 7, 2023

betatim approved these changes Aug 8, 2023

View reviewed changes

Merge branch 'main' into add-themes

bb1f1f6

adrinjalali merged commit 7d2da31 into scikit-learn:main Aug 9, 2023

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023

ENH Add themes for HTML display. Add dark theme (scikit-learn#26862)

17c4a59

lesteve mentioned this pull request Sep 13, 2023

ENH improve visual HTML estimator representation #26616

Merged

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

ENH Add themes for HTML display. Add dark theme (scikit-learn#26862)

154d4c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add themes for HTML display. Add dark theme #26862

ENH Add themes for HTML display. Add dark theme #26862

ENH Add themes for HTML display. Add dark theme #26862

ENH Add themes for HTML display. Add dark theme #26862

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

✔️ Linting Passed

Approach 1 - In Pipeline, add new optional attribute theme:

Approach 2 - In Config, modify existing optional attribute display:

Approach 3 - In Config, add new optional attribute theme:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Approach 1 - In Pipeline, add new optional attribute `theme`:

Approach 2 - In Config, modify existing optional attribute `display`:

Approach 3 - In Config, add new optional attribute `theme`: