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

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

Merged
merged 49 commits into from Aug 9, 2023

Conversation

9Y5
Copy link
Contributor

@9Y5 9Y5 commented Jul 19, 2023

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:
Screenshot 2023-07-21 at 11 58 25 AM

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:
Screenshot 2023-07-20 at 9 38 06 PM

When a theme such as themes.DARK is given, it can set the theme to dark theme:
Screenshot 2023-07-20 at 9 38 19 PM

Any other comments?

@github-actions
Copy link

github-actions bot commented Jul 19, 2023

✔️ Linting Passed

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

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

@thomasjpfan
Copy link
Member

thomasjpfan commented Jul 19, 2023

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
Copy link
Contributor Author

9Y5 commented Jul 19, 2023

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 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
@betatim
Copy link
Member

betatim commented Jul 20, 2023

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
Copy link
Contributor Author

9Y5 commented Jul 20, 2023

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:
Screenshot 2023-07-20 at 7 15 11 PM

@betatim
Copy link
Member

betatim commented Jul 20, 2023

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
Copy link
Contributor Author

9Y5 commented Jul 20, 2023

@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
Copy link
Contributor Author

9Y5 commented Jul 20, 2023

Put differently, this MR:

  1. Can resolve Adding dark mode for pipeline diagram #26364 and similar requests to style the pipeline diagram.
  2. 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
Copy link
Member

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
Copy link
Contributor Author

9Y5 commented Jul 20, 2023

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?

@9Y5 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
@adrinjalali
Copy link
Member

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
Copy link
Contributor Author

9Y5 commented Aug 1, 2023

@thomasjpfan @adrinjalali pls help review again, thank you

Looks like it can close #26364

@9Y5
Copy link
Contributor Author

9Y5 commented Aug 1, 2023

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.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

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

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

_STYLE = """
#$id {
color: black;
--sklearn-color-1: black;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 9Y5 force-pushed the add-themes branch 2 times, most recently from 5953ca8 to f0c5238 Compare August 2, 2023 16:15
@9Y5 9Y5 requested a review from thomasjpfan August 2, 2023 16:19
@9Y5 9Y5 force-pushed the add-themes branch 3 times, most recently from adcc5e2 to 11516c2 Compare August 2, 2023 16:56
@9Y5
Copy link
Contributor Author

9Y5 commented Aug 2, 2023

let me re-open, need to fix the branches

@9Y5
Copy link
Contributor Author

9Y5 commented Aug 2, 2023

Reopening

@9Y5 9Y5 deleted the add-themes branch August 2, 2023 17:20
@9Y5 9Y5 restored the add-themes branch August 2, 2023 17:20
@9Y5 9Y5 reopened this Aug 2, 2023
@9Y5
Copy link
Contributor Author

9Y5 commented Aug 5, 2023

@thomasjpfan @adrinjalali PR is ready for review again

@thomasjpfan 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
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

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

@betatim betatim left a comment

Choose a reason for hiding this comment

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

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

@adrinjalali adrinjalali merged commit 7d2da31 into scikit-learn:main Aug 9, 2023
27 checks passed
TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:utils Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants