v4.4.50 - Indicators hardening + ThetaData acceptance stabilization#967
Open
v4.4.50 - Indicators hardening + ThetaData acceptance stabilization#967
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded environment-driven controls and helpers for indicator plotting (dynamic subplot spacing, UI/html toggles, CSV/Parquet fallback on render failure), CI/acceptance-aware ThetaData intraday gap handling during prefetches, new tests covering these behaviors, and bumped package version to 4.4.50. Changes
Sequence DiagramssequenceDiagram
participant User
participant plot_indicators as plot_indicators()
participant env as Environment
participant subplots as make_subplots()
participant renderer as Plotly Renderer
participant html_writer as write_html()
participant fallback as CSV/Parquet Export
User->>plot_indicators: call with data & configs
plot_indicators->>env: read LUMIBOT_DISABLE_UI / LUMIBOT_WRITE_INDICATORS_HTML
env-->>plot_indicators: flags
plot_indicators->>subplots: create subplots (safe spacing)
subplots-->>plot_indicators: figure
plot_indicators->>renderer: render traces/hover/text
renderer-->>plot_indicators: rendered
alt HTML writing enabled
plot_indicators->>html_writer: write_html()
html_writer-->>plot_indicators: success
else HTML write fails or disabled
plot_indicators->>fallback: write CSV/Parquet artifacts
fallback-->>plot_indicators: artifacts produced
end
plot_indicators-->>User: return artifacts / logs
sequenceDiagram
participant Backtest
participant _covers_window as _covers_window()
participant env as Environment
participant logger as Logger
participant data as DataFetcher
Backtest->>_covers_window: validate coverage for window
_covers_window->>data: detect intraday index gap
data-->>_covers_window: gap found
_covers_window->>env: check LUMIBOT_ACCEPTANCE_BACKTEST
alt Acceptance/CI enabled
env-->>_covers_window: true
_covers_window->>logger: log warning + diagnostics
_covers_window->>Backtest: continue with available data (no raise)
else Acceptance disabled
env-->>_covers_window: false
_covers_window-->>Backtest: raise ValueError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 3-5: Replace the placeholder under the heading "## 4.4.50 -
Unreleased" with a concrete changelog entry that lists the two PR changes: the
subplot spacing fix and the non-fatal HTML export behavior; mention the affected
features (subplot spacing adjustment and HTML export error handling), include a
brief one-line description for each change and any user-facing impact or
migration notes, and ensure the version header "## 4.4.50 - Unreleased" is
preserved.
In `@lumibot/tools/indicators.py`:
- Around line 549-805: The try/except is too broad and currently wraps all
plotting logic (functions like
generate_marker_plotly_text/_generate_ohlc_hover_text, loops over
chart_markers_df, chart_lines_df, chart_ohlc_df, and fig construction), so move
the try/except so only the HTML export call (fig.write_html(...)) is wrapped;
let the subplot creation, trace additions, color/size handling, and layout code
run outside the try so data-processing errors propagate normally, and keep the
existing logger.exception message around the fig.write_html call to make HTML
export non-fatal.
🧹 Nitpick comments (3)
lumibot/tools/indicators.py (2)
453-464: LUMIBOT_DISABLE_UI was explicitly removed in v4.4.31 — re-introducing it may confuse users.The CHANGELOG for 4.4.31 states: "Removed the short-lived
LUMIBOT_DISABLE_UIenv var (useSHOW_PLOT/SHOW_INDICATORS/SHOW_TEARSHEET+ pytest non-interactive behavior instead)." This code re-readsLUMIBOT_DISABLE_UIat lines 792, 879, and 1534 via the new_env_flag_enabledhelper, effectively resurrecting the removed variable.Additionally,
LUMIBOT_WRITE_INDICATORS_HTMLis a new env var. Based on learnings: "Do not add new environment variables by default. Prefer explicit function parameters, config objects, or stable defaults."Consider either:
- Relying solely on
PYTEST_CURRENT_TEST+ the existingshow_*parameters (already passed through), or- If the env vars are truly needed, documenting them in
docsrc/environment_variables.rstas per your coding guidelines.
543-547: Nit: the log message assumes the original default is always 0.15.If
_safe_subplot_vertical_spacingis later called with a differentdefault_spacing, this log line would be misleading. Minor, but worth noting.tests/test_indicator_subplots.py (1)
314-315: Redundant local import —plot_indicatorsis already imported at line 12.Suggested fix
def test_plot_indicators_handles_nan_marker_size(tmp_path, monkeypatch): - from lumibot.tools.indicators import plot_indicators - # Build a marker DataFrame with NaN sizes to mirror the failing scenario
CHANGELOG.md
Outdated
Comment on lines
3
to
5
| ## 4.4.50 - Unreleased | ||
|
|
||
| - TBD |
There was a problem hiding this comment.
Changelog entry is a placeholder — fill in the actual changes before release.
The PR introduces two concrete changes (subplot spacing fix, non-fatal HTML export), but the changelog only contains "TBD". Per the coding guidelines, the changelog must document all changes in a version. Suggested content:
Proposed changelog entry
## 4.4.50 - Unreleased
-- TBD
+### Fixed
+- Indicators: fix subplot vertical spacing overflow for charts with many indicator rows (Plotly requires `vertical_spacing <= 1/(rows-1)`).
+- Indicators: make HTML chart generation non-fatal — emit a warning and continue with CSV/Parquet export when rendering fails.As per coding guidelines: "The changelog at CHANGELOG.md MUST be updated for every deployment, release, or significant change. The changelog documents ALL changes in a version."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## 4.4.50 - Unreleased | |
| - TBD | |
| ## 4.4.50 - Unreleased | |
| ### Fixed | |
| - Indicators: fix subplot vertical spacing overflow for charts with many indicator rows (Plotly requires `vertical_spacing <= 1/(rows-1)`). | |
| - Indicators: make HTML chart generation non-fatal — emit a warning and continue with CSV/Parquet export when rendering fails. |
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 3 - 5, Replace the placeholder under the heading
"## 4.4.50 - Unreleased" with a concrete changelog entry that lists the two PR
changes: the subplot spacing fix and the non-fatal HTML export behavior; mention
the affected features (subplot spacing adjustment and HTML export error
handling), include a brief one-line description for each change and any
user-facing impact or migration notes, and ensure the version header "## 4.4.50
- Unreleased" is preserved.
Comment on lines
+549
to
+805
| try: | ||
| # Create subplots without shared x-axes | ||
| fig = make_subplots( | ||
| rows=num_subplots, | ||
| cols=1, | ||
| subplot_titles=subplot_titles, | ||
| shared_xaxes=False, # Do not use shared x-axes | ||
| vertical_spacing=vertical_spacing, | ||
| ) | ||
|
|
||
| # Plot the chart markers | ||
| if chart_markers_df is not None and not chart_markers_df.empty: | ||
| chart_markers_df["detail_text"] = chart_markers_df.apply(generate_marker_plotly_text, axis=1) | ||
|
|
||
| # Group by plot_name first, then by name | ||
| for plot_name, plot_df in chart_markers_df.groupby("plot_name"): | ||
| # Loop over the marker names for this plot_name | ||
| for marker_name, group_df in plot_df.groupby("name"): | ||
| group_df = group_df.copy() | ||
| # Get the marker symbol | ||
| marker_symbol = group_df["symbol"].iloc[0] | ||
|
|
||
| # Determine marker size(s), falling back to sensible defaults when unspecified | ||
| default_marker_size = 25 | ||
| raw_sizes = group_df.get("size") | ||
| marker_size = default_marker_size | ||
|
|
||
| if raw_sizes is not None: | ||
| marker_sizes = pd.to_numeric(raw_sizes, errors="coerce") | ||
|
|
||
| if isinstance(marker_sizes, pd.Series): | ||
| marker_sizes = marker_sizes.fillna(default_marker_size).clip(lower=1) | ||
| unique_sizes = marker_sizes.unique() | ||
| if len(unique_sizes) == 1: | ||
| marker_size = float(unique_sizes[0]) | ||
| else: | ||
| marker_size = marker_sizes.tolist() | ||
| else: | ||
| if pd.isna(marker_sizes) or marker_sizes <= 0: | ||
| marker_size = default_marker_size | ||
| has_chart_data = False | ||
|
|
||
| ############################### | ||
| # Chart Markers | ||
| ############################### | ||
|
|
||
| def generate_marker_plotly_text(row): | ||
| return _format_indicator_plotly_text(row.get("value"), row.get("detail_text")) | ||
|
|
||
| # Plot the chart markers | ||
| if chart_markers_df is not None and not chart_markers_df.empty: | ||
| chart_markers_df["detail_text"] = chart_markers_df.apply(generate_marker_plotly_text, axis=1) | ||
|
|
||
| # Group by plot_name first, then by name | ||
| for plot_name, plot_df in chart_markers_df.groupby("plot_name"): | ||
| # Loop over the marker names for this plot_name | ||
| for marker_name, group_df in plot_df.groupby("name"): | ||
| group_df = group_df.copy() | ||
| # Get the marker symbol | ||
| marker_symbol = group_df["symbol"].iloc[0] | ||
|
|
||
| # Determine marker size(s), falling back to sensible defaults when unspecified | ||
| default_marker_size = 25 | ||
| raw_sizes = group_df.get("size") | ||
| marker_size = default_marker_size | ||
|
|
||
| if raw_sizes is not None: | ||
| marker_sizes = pd.to_numeric(raw_sizes, errors="coerce") | ||
|
|
||
| if isinstance(marker_sizes, pd.Series): | ||
| marker_sizes = marker_sizes.fillna(default_marker_size).clip(lower=1) | ||
| unique_sizes = marker_sizes.unique() | ||
| if len(unique_sizes) == 1: | ||
| marker_size = float(unique_sizes[0]) | ||
| else: | ||
| marker_size = marker_sizes.tolist() | ||
| else: | ||
| marker_size = float(marker_sizes) | ||
|
|
||
| if "color" not in group_df.columns: | ||
| group_df["color"] = None | ||
| group_df.loc[:, "color"] = group_df["color"].apply( | ||
| lambda val: _safe_color(val, f"{plot_name}:{marker_name}") | ||
| ) | ||
|
|
||
| # Determine which subplot to use | ||
| row = plot_names.index(plot_name) + 1 | ||
|
|
||
| # Create a new trace for this marker name | ||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=group_df["datetime"], | ||
| y=group_df["value"], | ||
| mode="markers", | ||
| name=marker_name, | ||
| marker_color=group_df["color"], | ||
| marker_size=marker_size, | ||
| marker_symbol=marker_symbol, | ||
| hovertemplate=f"{marker_name}<br>%{{text}}<br>%{{x|%b %d %Y %I:%M:%S %p}}<extra></extra>", | ||
| text=group_df["detail_text"], | ||
| ), | ||
| row=row, | ||
| col=1 | ||
| ) | ||
|
|
||
| has_chart_data = True | ||
|
|
||
| ############################### | ||
| # Chart Lines | ||
| ############################### | ||
|
|
||
| def generate_line_plotly_text(row): | ||
| return _format_indicator_plotly_text(row.get("value"), row.get("detail_text")) | ||
|
|
||
| # Plot the chart lines | ||
| if chart_lines_df is not None and not chart_lines_df.empty: | ||
| chart_lines_df["detail_text"] = chart_lines_df.apply(generate_line_plotly_text, axis=1) | ||
|
|
||
| # Group by plot_name first, then by name | ||
| for plot_name, plot_df in chart_lines_df.groupby("plot_name"): | ||
| # Loop over the line names for this plot_name | ||
| for line_name, group_df in plot_df.groupby("name"): | ||
| if "color" not in group_df.columns: | ||
| group_df = group_df.assign(color=None) | ||
| color = _safe_color(group_df["color"].iloc[0], f"{plot_name}:{line_name}") | ||
|
|
||
| # Determine which subplot to use | ||
| row = plot_names.index(plot_name) + 1 | ||
|
|
||
| # Create a new trace for this line name | ||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=group_df["datetime"], | ||
| y=group_df["value"], | ||
| mode="lines", | ||
| name=line_name, | ||
| line_color=color, | ||
| hovertemplate=f"{line_name}<br>%{{text}}<br>%{{x|%b %d %Y %I:%M:%S %p}}<extra></extra>", | ||
| text=group_df["detail_text"], | ||
| ), | ||
| row=row, | ||
| col=1 | ||
| ) | ||
|
|
||
| has_chart_data = True | ||
| if pd.isna(marker_sizes) or marker_sizes <= 0: | ||
| marker_size = default_marker_size | ||
| else: | ||
| marker_size = float(marker_sizes) | ||
|
|
||
| if "color" not in group_df.columns: | ||
| group_df["color"] = None | ||
| group_df.loc[:, "color"] = group_df["color"].apply( | ||
| lambda val: _safe_color(val, f"{plot_name}:{marker_name}") | ||
| ) | ||
|
|
||
| ############################### | ||
| # Chart OHLC | ||
| ############################### | ||
| # Determine which subplot to use | ||
| row = plot_names.index(plot_name) + 1 | ||
|
|
||
| def _generate_ohlc_hover_text(row): | ||
| base = f"O: {row['open']}<br>H: {row['high']}<br>L: {row['low']}<br>C: {row['close']}" | ||
| if row.get("detail_text") is None: | ||
| return base | ||
| return base + "<br>" + str(row.get("detail_text")) | ||
| # Create a new trace for this marker name | ||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=group_df["datetime"], | ||
| y=group_df["value"], | ||
| mode="markers", | ||
| name=marker_name, | ||
| marker_color=group_df["color"], | ||
| marker_size=marker_size, | ||
| marker_symbol=marker_symbol, | ||
| hovertemplate=f"{marker_name}<br>%{{text}}<br>%{{x|%b %d %Y %I:%M:%S %p}}<extra></extra>", | ||
| text=group_df["detail_text"], | ||
| ), | ||
| row=row, | ||
| col=1 | ||
| ) | ||
|
|
||
| if chart_ohlc_df is not None and not chart_ohlc_df.empty: | ||
| chart_ohlc_df = chart_ohlc_df.copy() | ||
| has_chart_data = True | ||
|
|
||
| for col in ("open", "high", "low", "close"): | ||
| if col not in chart_ohlc_df.columns: | ||
| logger.warning(f"OHLC data missing required column '{col}', skipping OHLC plotting.") | ||
| chart_ohlc_df = None | ||
| break | ||
| ############################### | ||
| # Chart Lines | ||
| ############################### | ||
|
|
||
| if chart_ohlc_df is not None and not chart_ohlc_df.empty: | ||
| if "color" not in chart_ohlc_df.columns: | ||
| chart_ohlc_df["color"] = None | ||
| def generate_line_plotly_text(row): | ||
| return _format_indicator_plotly_text(row.get("value"), row.get("detail_text")) | ||
|
|
||
| # Default per-bar colors: green for bullish, red for bearish (matches Strategy.add_ohlc defaults). | ||
| chart_ohlc_df["color"] = chart_ohlc_df["color"].where( | ||
| chart_ohlc_df["color"].notna(), | ||
| np.where(chart_ohlc_df["close"] >= chart_ohlc_df["open"], "green", "red"), | ||
| ) | ||
| # Plot the chart lines | ||
| if chart_lines_df is not None and not chart_lines_df.empty: | ||
| chart_lines_df["detail_text"] = chart_lines_df.apply(generate_line_plotly_text, axis=1) | ||
|
|
||
| chart_ohlc_df["detail_text"] = chart_ohlc_df.apply(_generate_ohlc_hover_text, axis=1) | ||
| # Group by plot_name first, then by name | ||
| for plot_name, plot_df in chart_lines_df.groupby("plot_name"): | ||
| # Loop over the line names for this plot_name | ||
| for line_name, group_df in plot_df.groupby("name"): | ||
| if "color" not in group_df.columns: | ||
| group_df = group_df.assign(color=None) | ||
| color = _safe_color(group_df["color"].iloc[0], f"{plot_name}:{line_name}") | ||
|
|
||
| # Group by plot_name first, then by series name. | ||
| for plot_name, plot_df in chart_ohlc_df.groupby("plot_name"): | ||
| for ohlc_name, group_df in plot_df.groupby("name"): | ||
| # Determine which subplot to use | ||
| row = plot_names.index(plot_name) + 1 | ||
|
|
||
| # Preserve per-bar colors by splitting into separate traces per color. | ||
| color_groups = list(group_df.groupby("color")) | ||
| for idx, (bar_color, colored_df) in enumerate(color_groups): | ||
| trace_color = _safe_color(bar_color, f"{plot_name}:{ohlc_name}:{bar_color}") | ||
|
|
||
| fig.add_trace( | ||
| go.Candlestick( | ||
| x=colored_df["datetime"], | ||
| open=colored_df["open"], | ||
| high=colored_df["high"], | ||
| low=colored_df["low"], | ||
| close=colored_df["close"], | ||
| name=ohlc_name, | ||
| showlegend=idx == 0, | ||
| legendgroup=ohlc_name, | ||
| increasing_line_color=trace_color, | ||
| decreasing_line_color=trace_color, | ||
| increasing_fillcolor=trace_color, | ||
| decreasing_fillcolor=trace_color, | ||
| hovertext=colored_df["detail_text"], | ||
| hoverinfo="x+text", | ||
| ), | ||
| row=row, | ||
| col=1, | ||
| ) | ||
| # Create a new trace for this line name | ||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=group_df["datetime"], | ||
| y=group_df["value"], | ||
| mode="lines", | ||
| name=line_name, | ||
| line_color=color, | ||
| hovertemplate=f"{line_name}<br>%{{text}}<br>%{{x|%b %d %Y %I:%M:%S %p}}<extra></extra>", | ||
| text=group_df["detail_text"], | ||
| ), | ||
| row=row, | ||
| col=1 | ||
| ) | ||
|
|
||
| has_chart_data = True | ||
|
|
||
| ############################### | ||
| # Chart Titles and Layouts | ||
| ############################### | ||
| ############################### | ||
| # Chart OHLC | ||
| ############################### | ||
|
|
||
| # Set title and layout | ||
| # Calculate height based on number of subplots | ||
| # 400px per subplot | ||
| height = max(800, num_subplots * 400) | ||
| def _generate_ohlc_hover_text(row): | ||
| base = f"O: {row['open']}<br>H: {row['high']}<br>L: {row['low']}<br>C: {row['close']}" | ||
| if row.get("detail_text") is None: | ||
| return base | ||
| return base + "<br>" + str(row.get("detail_text")) | ||
|
|
||
| title_text = f"Indicators for {strategy_name}" if strategy_name else "Indicators" | ||
| if not has_chart_data: | ||
| title_text = title_text + " (no indicator data)" | ||
| if chart_ohlc_df is not None and not chart_ohlc_df.empty: | ||
| chart_ohlc_df = chart_ohlc_df.copy() | ||
|
|
||
| for col in ("open", "high", "low", "close"): | ||
| if col not in chart_ohlc_df.columns: | ||
| logger.warning(f"OHLC data missing required column '{col}', skipping OHLC plotting.") | ||
| chart_ohlc_df = None | ||
| break | ||
|
|
||
| if chart_ohlc_df is not None and not chart_ohlc_df.empty: | ||
| if "color" not in chart_ohlc_df.columns: | ||
| chart_ohlc_df["color"] = None | ||
|
|
||
| # Default per-bar colors: green for bullish, red for bearish (matches Strategy.add_ohlc defaults). | ||
| chart_ohlc_df["color"] = chart_ohlc_df["color"].where( | ||
| chart_ohlc_df["color"].notna(), | ||
| np.where(chart_ohlc_df["close"] >= chart_ohlc_df["open"], "green", "red"), | ||
| ) | ||
|
|
||
| fig.update_layout( | ||
| title_text=title_text, | ||
| title_font_size=30, | ||
| template="plotly_dark", | ||
| height=height, # Dynamic height based on number of subplots | ||
| margin=dict(t=150), # Add more space between title and first subplot | ||
| ) | ||
| chart_ohlc_df["detail_text"] = chart_ohlc_df.apply(_generate_ohlc_hover_text, axis=1) | ||
|
|
||
| # Group by plot_name first, then by series name. | ||
| for plot_name, plot_df in chart_ohlc_df.groupby("plot_name"): | ||
| for ohlc_name, group_df in plot_df.groupby("name"): | ||
| row = plot_names.index(plot_name) + 1 | ||
|
|
||
| # Preserve per-bar colors by splitting into separate traces per color. | ||
| color_groups = list(group_df.groupby("color")) | ||
| for idx, (bar_color, colored_df) in enumerate(color_groups): | ||
| trace_color = _safe_color(bar_color, f"{plot_name}:{ohlc_name}:{bar_color}") | ||
|
|
||
| fig.add_trace( | ||
| go.Candlestick( | ||
| x=colored_df["datetime"], | ||
| open=colored_df["open"], | ||
| high=colored_df["high"], | ||
| low=colored_df["low"], | ||
| close=colored_df["close"], | ||
| name=ohlc_name, | ||
| showlegend=idx == 0, | ||
| legendgroup=ohlc_name, | ||
| increasing_line_color=trace_color, | ||
| decreasing_line_color=trace_color, | ||
| increasing_fillcolor=trace_color, | ||
| decreasing_fillcolor=trace_color, | ||
| hovertext=colored_df["detail_text"], | ||
| hoverinfo="x+text", | ||
| ), | ||
| row=row, | ||
| col=1, | ||
| ) | ||
|
|
||
| has_chart_data = True | ||
|
|
||
| ############################### | ||
| # Chart Titles and Layouts | ||
| ############################### | ||
|
|
||
| # Set title and layout | ||
| # Calculate height based on number of subplots | ||
| # 400px per subplot | ||
| height = max(800, num_subplots * 400) | ||
|
|
||
| title_text = f"Indicators for {strategy_name}" if strategy_name else "Indicators" | ||
| if not has_chart_data: | ||
| title_text = title_text + " (no indicator data)" | ||
|
|
||
| fig.update_layout( | ||
| title_text=title_text, | ||
| title_font_size=30, | ||
| template="plotly_dark", | ||
| height=height, # Dynamic height based on number of subplots | ||
| margin=dict(t=150), # Add more space between title and first subplot | ||
| ) | ||
|
|
||
| if has_chart_data: | ||
| # Range selector buttons | ||
| rangeselector_buttons = list([ | ||
| dict(count=1, label="1m", step="month", stepmode="backward"), | ||
| dict(count=6, label="6m", step="month", stepmode="backward"), | ||
| dict(count=1, label="YTD", step="year", stepmode="todate"), | ||
| dict(count=1, label="1y", step="year", stepmode="backward"), | ||
| dict(step="all"), | ||
| ]) | ||
|
|
||
| # Update axes for all subplots | ||
| for i in range(1, num_subplots + 1): | ||
| # Get the plot name for this subplot | ||
| plot_title = plot_names[i - 1] | ||
|
|
||
| # Set y-axes titles for each subplot | ||
| fig.update_yaxes( | ||
| title_text=plot_title, | ||
| secondary_y=False, | ||
| row=i, | ||
| col=1 | ||
| ) | ||
| if has_chart_data: | ||
| # Range selector buttons | ||
| rangeselector_buttons = list([ | ||
| dict(count=1, label="1m", step="month", stepmode="backward"), | ||
| dict(count=6, label="6m", step="month", stepmode="backward"), | ||
| dict(count=1, label="YTD", step="year", stepmode="todate"), | ||
| dict(count=1, label="1y", step="year", stepmode="backward"), | ||
| dict(step="all"), | ||
| ]) | ||
|
|
||
| # Update axes for all subplots | ||
| for i in range(1, num_subplots + 1): | ||
| # Get the plot name for this subplot | ||
| plot_title = plot_names[i - 1] | ||
|
|
||
| # Set y-axes titles for each subplot | ||
| fig.update_yaxes( | ||
| title_text=plot_title, | ||
| secondary_y=False, | ||
| row=i, | ||
| col=1 | ||
| ) | ||
|
|
||
| # Add range selector and range slider to each subplot | ||
| fig.update_xaxes( | ||
| rangeselector=dict( | ||
| buttons=rangeselector_buttons, | ||
| font=dict(color="black"), | ||
| activecolor="grey", | ||
| bgcolor="white", | ||
| ), | ||
| rangeslider=dict( | ||
| visible=True, | ||
| thickness=0.02 # Make the range slider height shorter to make line graph appear taller | ||
| ), | ||
| row=i, | ||
| col=1 | ||
| ) | ||
| # Add range selector and range slider to each subplot | ||
| fig.update_xaxes( | ||
| rangeselector=dict( | ||
| buttons=rangeselector_buttons, | ||
| font=dict(color="black"), | ||
| activecolor="grey", | ||
| bgcolor="white", | ||
| ), | ||
| rangeslider=dict( | ||
| visible=True, | ||
| thickness=0.02 # Make the range slider height shorter to make line graph appear taller | ||
| ), | ||
| row=i, | ||
| col=1 | ||
| ) | ||
|
|
||
| disable_ui = ( | ||
| os.environ.get("LUMIBOT_DISABLE_UI", "").strip().lower() in ("1", "true", "yes") | ||
| or bool(os.environ.get("PYTEST_CURRENT_TEST")) | ||
| ) | ||
| disable_ui = _env_flag_enabled("LUMIBOT_DISABLE_UI", default=False) or bool(os.environ.get("PYTEST_CURRENT_TEST")) | ||
| write_indicators_html = _env_flag_enabled("LUMIBOT_WRITE_INDICATORS_HTML", default=True) | ||
|
|
||
| # Create graph (auto_open disabled for CI/tests). | ||
| fig.write_html(plot_file_html, auto_open=show_indicators and not disable_ui) | ||
| if write_indicators_html: | ||
| # Create graph (auto_open disabled for CI/tests). | ||
| fig.write_html(plot_file_html, auto_open=show_indicators and not disable_ui) | ||
| else: | ||
| logger.info( | ||
| "Skipping indicators HTML generation because LUMIBOT_WRITE_INDICATORS_HTML is disabled." | ||
| ) | ||
| except Exception: | ||
| logger.exception( | ||
| "Indicators subplot rendering failed; continuing with indicators CSV/parquet export." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The try/except scope is very broad — consider narrowing it to just the HTML write.
The try block wraps ~250 lines of plotting logic (subplot creation, all trace additions, layout configuration, and the HTML write). Any bug in marker/line/OHLC data processing (e.g., a KeyError, type mismatch) will be silently swallowed with only a log message, making debugging difficult.
The PR objective is to make HTML export non-fatal. Consider narrowing the try/except to wrap only fig.write_html(...) (line 797), while letting data-processing errors propagate normally.
Sketch of narrower scope
- try:
- # Create subplots without shared x-axes
- fig = make_subplots(
- ...
- )
- ... # ~250 lines of plotting logic
- if write_indicators_html:
- fig.write_html(plot_file_html, auto_open=show_indicators and not disable_ui)
- ...
- except Exception:
- logger.exception(
- "Indicators subplot rendering failed; continuing with indicators CSV/parquet export."
- )
+ # Create subplots without shared x-axes
+ fig = make_subplots(
+ ...
+ )
+ ... # plotting logic (errors propagate normally)
+
+ if write_indicators_html:
+ try:
+ fig.write_html(plot_file_html, auto_open=show_indicators and not disable_ui)
+ except Exception:
+ logger.exception(
+ "Indicators HTML write failed; continuing with CSV/parquet export."
+ )🤖 Prompt for AI Agents
In `@lumibot/tools/indicators.py` around lines 549 - 805, The try/except is too
broad and currently wraps all plotting logic (functions like
generate_marker_plotly_text/_generate_ohlc_hover_text, loops over
chart_markers_df, chart_lines_df, chart_ohlc_df, and fig construction), so move
the try/except so only the HTML export call (fig.write_html(...)) is wrapped;
let the subplot creation, trace additions, color/size handling, and layout code
run outside the try so data-processing errors propagate normally, and keep the
existing logger.exception message around the fig.write_html call to make HTML
export non-fatal.
grzesir
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this ships
Validation