-
Notifications
You must be signed in to change notification settings - Fork 872
Allow execution without plotly #2401
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
Conversation
MaartenGr
left a comment
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.
Thank you for the PR! I left some comments here and there. Most importantly, I think this solution is more complex than is currently needed. We only need to mock the plotting as we can do the type-hinting of go.Figure like so: "go.Figure".
As an example, we can reduce the complexity to this:
import importlib.util
import logging
from typing import Any
class MockPlotlyModule:
"""Mock module that raises informative errors when plotly functions are called."""
def __getattr__(self, name: str) -> Any:
def mock_function(*args, **kwargs):
raise ImportError(
f"Plotly is required to use '{name}'. "
"Install it with: pip install plotly"
)
return mock_function
# Clean import logic using importlib
if importlib.util.find_spec("plotly") is None:
logger.warning("Plotly is not installed. Plotting functions will raise errors when called.")
# Mock modules
plotting, go = MockPlotlyModule(), None
else:
from bertopic import plotting
import plotly.graph_objects as goI'm also wondering if we should do the logger.warning here as we cannot control that verbosity. There might be use-cases where you do not want that logging.
bertopic/_bertopic.py
Outdated
|
|
||
| except ModuleNotFoundError as e: | ||
| if "No module named 'plotly'" in str(e): | ||
| logger.warning("Plotly is not installed. Please install it to use the plotting functions.") |
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.
Perhaps also add a way to install it? A one-liner would suffice, something like "For example with pip install plotly or uv pip install plotly". That way, users would not have to search how to install plotly but can do it immediately.
bertopic/_bertopic.py
Outdated
| import plotly.graph_objects as go | ||
|
|
||
| except ModuleNotFoundError as e: | ||
| if "No module named 'plotly'" in str(e): |
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.
I'm wondering whether this is a good enough check to see whether plotly is installed. Checking the string might be unstable. Perhaps use importlib instead?
plotly_available = importlib.util.find_spec("plotly") is not None
bertopic/_utils.py
Outdated
| def __getattr__(self, name): | ||
| def mock_function(*args, **kwargs): | ||
| self.logger.warning(f"Plotly is not installed. Cannot use {name} visualization function.") | ||
| return MockFigure() | ||
|
|
||
| return mock_function |
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.
Let's simplify this by raising an error instead.
bertopic/_utils.py
Outdated
| class MockFigure: | ||
| """Mock class for plotly.graph_objects.Figure when plotly is not installed.""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| pass |
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.
I don't think this is necessary when we can use def some_function(self) -> "go.Figure" instead. This would reduce the code quite a bit.
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.
Good call. Will use plotly.graph_objs instead of plotly.graph_objects because only the later is available when typing.TYPE_CHECKING is True.
I like this overall approach, simpler indeed. The visualize methods now return string literal type hint "go.Figure" which works fine (as far as the type checker goes) in both cases - with and without plotly installed. I've also gone ahead and fixed
Fine. For consistency, the check for HDBSCAN on line 41 should use something similar.
I'm ok with removing that logger.warning - there is already a clear exception if the user tries to use visualizations without plotly present. Also, ideally BERTopic should have a way to configure the log level more dynamically rather than the hardcoded WARNING level. |
|
Apologies for the delay and thank you for the updated code. I agree with the logger, it should be handled a bit more elegantly than it is now (mostly handled in the main class and not easily changed). Either way, I'll go ahead and merge this, so thank you for your work on this. It is greatly appreciated! |
Allow execution without plotly.
Fixes #2398
Handles calls to .visualize... methods with a
passand a logger.warningBefore submitting