-
Notifications
You must be signed in to change notification settings - Fork 16
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 HTML to present warnings of train_test_split when in notebooks #1060
Conversation
It might be fine temporary. My short term vision is that those warnings should be collected and added the |
Thanks, could we also display the title of the warning such as |
Coverage Report for backend
|
Thanks! LGTM What if there are several warnings? |
if is_environment_html_capable(): | ||
with contextlib.suppress(ImportError): | ||
from IPython.core.interactiveshell import InteractiveShell | ||
from IPython.display import HTML, display | ||
from markdown import markdown | ||
|
||
if InteractiveShell.initialized(): | ||
markup = "".join( | ||
[ | ||
HTML_WARNING_TEMPLATE.format( | ||
warning=markdown(warning), | ||
warning_class=re.sub( | ||
"([A-Z][a-z]+)", | ||
r" \1", | ||
re.sub("([A-Z]+)", r" \1", warning_class.__name__), | ||
).lstrip(), | ||
) | ||
for warning, warning_class in to_display | ||
] | ||
) | ||
display(HTML(markup)) | ||
else: | ||
for warning, warning_class in to_display: | ||
warnings.warn(message=warning, category=warning_class, stacklevel=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative that would work in both HTML and in prompt would be to leverage rich
:
from skore import console # avoid circular import
from rich.panel import Panel
for warning_class in TRAIN_TEST_SPLIT_WARNINGS:
warning = warning_class.check(**kwargs)
if warning is not None:
# Only check if warning is filtered/ignored
if not warnings.filters or not any(
f[0] == "ignore" and f[2] == warning_class for f in warnings.filters
):
console.print(
Panel(
title=warning_class.__name__,
renderable=warning,
style="orange1",
border_style="cyan",
)
)
It would provide the following outputs:
And here it is configure such that someone filter the warnings with ignore, then we don't show it.
It might be a bit less invasive than checking for the environment (while this code could be useful for some other stuff along the road).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using rich everywhere does provide coherence (with the EstimatorReport of #997)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can rich display False
with code style for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can rich display
False
with code style for example?
Yes rich is capable to display markdown.
I hadn't considered using pure textual, it's cool and lightweight. Only down side is that we cant match hosting styles.
I'm happy to refactor that way. @MarieS-WiMLDS can you please settle this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by matching hosting style? Is it light/dark theme, or is it about the font?
I really don't mind if we don't have the default font of the user, it's more troublesome if it's about the theme, because it might affect readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An note that we have a similar limitation with the logging when creating project, etc. where we use rich
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your insights, anyway an orange background seems anti-UX, let's move on with rich. Should we close this PR and open a new one for rich? How do you wish to proceed with rich?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Sylvain, very unlikely for a user to have a orange background!
The "we can do it later" stuff tends to be never done, especially when it's about having clean code, so I'd prefer that we go for rich before merging, if it's compatible with your deadline of tuesday evening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf. #1086
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're the best ⚡️
This PR modifies the behavior of
train_test_split
.From now on, in a notebook context (vscode or jupyter), messages will no longer be warnings but HTML, in order to avoid horizontal scrolling.
Why do this?
The
train_test_split
feature is dedicated to an experimentation phase and has an educational purpose. We anticipate that most of our users will use notebooks or an interactive context. Some will disable warnings...Therefore, I believe that warning messages are not the right communication medium during the experimentation phase. Additionally, it's not very visually appealing. While waiting to create a proper
TrainTestSplitReport
, this PR proposes an alternative.UI preview
jupyter.mp4
vscode.mp4