Add Alpaca live broker strategy CI - #1104
Closed
mpelteshki wants to merge 9 commits into
Closed
Conversation
📝 WalkthroughWalkthroughAdds an Alpaca live-broker workflow and test gating, plus data/backtesting refactors for bar retrieval, duplicate handling, and order reconciliation with matching regression coverage and docs. ChangesAlpaca live broker CI and tests
Data and backtesting performance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
.github/workflows/alpaca-live-broker.yml (2)
48-48: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.Static analysis (zizmor) flags credential persistence via
actions/checkout(artipacked). Since no later step needs to push using the git token, disable persistence.🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/alpaca-live-broker.yml at line 48, The checkout step in the Alpaca live broker workflow is persisting git credentials unnecessarily. Update the existing actions/checkout usage to disable credential persistence by setting persist-credentials to false, since no later step needs to push with the token.Source: Linters/SAST tools
41-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a concurrency group to prevent overlapping live-broker runs.
This job submits and cancels real orders against a shared Alpaca paper account. Without a
concurrencyblock, simultaneous pushes/PRs (or a push racing a manual dispatch) could run in parallel and interfere with each other's orders (e.g., one run'scancel_open_orders()cancelling another run's in-flight order), causing flaky failures.♻️ Proposed concurrency group
jobs: alpaca-live-broker: name: Alpaca live broker strategy runs-on: blacksmith-2vcpu-ubuntu-2404 timeout-minutes: 30 if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + concurrency: + group: alpaca-live-broker + cancel-in-progress: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/alpaca-live-broker.yml around lines 41 - 46, The alpaca-live-broker job is missing a concurrency safeguard, so overlapping runs can interfere with shared Alpaca paper-account orders. Add a concurrency block to the alpaca-live-broker workflow job and use a stable group key tied to the workflow/job and branch or ref so only one live-broker execution runs at a time. Make sure the new setting is placed alongside the existing alpaca-live-broker job definition and preserves the current job gating in github.event_name and github.event.pull_request checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_alpaca_live_broker_apitest.py`:
- Around line 1-184: The new live-broker CI test coverage in the Alpaca apitest
module needs matching documentation updates. Add an engineering note in docs/
describing the new live-broker CI workflow, what _require_alpaca,
_LiveOrderDataStrategy, and _LiveOptionsChainStrategy validate, and any CI/env
requirements. Also update docsrc/ with any user-facing testing guidance changes
so contributors know how to run or interpret the live-broker tests.
---
Nitpick comments:
In @.github/workflows/alpaca-live-broker.yml:
- Line 48: The checkout step in the Alpaca live broker workflow is persisting
git credentials unnecessarily. Update the existing actions/checkout usage to
disable credential persistence by setting persist-credentials to false, since no
later step needs to push with the token.
- Around line 41-46: The alpaca-live-broker job is missing a concurrency
safeguard, so overlapping runs can interfere with shared Alpaca paper-account
orders. Add a concurrency block to the alpaca-live-broker workflow job and use a
stable group key tied to the workflow/job and branch or ref so only one
live-broker execution runs at a time. Make sure the new setting is placed
alongside the existing alpaca-live-broker job definition and preserves the
current job gating in github.event_name and github.event.pull_request checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ff4f70b-8a96-4c9f-bf4e-83bdd84804b3
📒 Files selected for processing (2)
.github/workflows/alpaca-live-broker.ymltests/test_alpaca_live_broker_apitest.py
Comment on lines
+1
to
+184
| import time | ||
| from datetime import datetime, timedelta | ||
|
|
||
| import pytest | ||
|
|
||
| from lumibot.brokers.alpaca import Alpaca | ||
| from lumibot.components.options_helper import OptionsHelper | ||
| from lumibot.credentials import ALPACA_TEST_CONFIG | ||
| from lumibot.entities import Asset, Order | ||
| from lumibot.strategies.strategy import Strategy | ||
|
|
||
| pytestmark = pytest.mark.apitest | ||
|
|
||
|
|
||
| def _require_alpaca() -> Alpaca: | ||
| api_key = ALPACA_TEST_CONFIG.get("API_KEY") | ||
| api_secret = ALPACA_TEST_CONFIG.get("API_SECRET") | ||
| if not api_key or not api_secret or api_key == "<your key here>" or api_secret == "<your key here>": | ||
| pytest.skip("Missing ALPACA_TEST_API_KEY / ALPACA_TEST_API_SECRET") | ||
|
|
||
| broker = Alpaca(ALPACA_TEST_CONFIG, max_workers=1, connect_stream=False) | ||
| try: | ||
| account = broker.api.get_account() | ||
| except Exception as exc: | ||
| broker.cleanup_streams() | ||
| raise RuntimeError(f"Alpaca paper account authentication failed: {exc}") from exc | ||
|
|
||
| if getattr(account, "trading_blocked", False): | ||
| broker.cleanup_streams() | ||
| pytest.skip("Alpaca paper account is trading-blocked") | ||
|
|
||
| return broker | ||
|
|
||
|
|
||
| def _status_text(raw_order) -> str: | ||
| status = getattr(raw_order, "status", "") | ||
| if hasattr(status, "value"): | ||
| status = status.value | ||
| return str(status).lower() | ||
|
|
||
|
|
||
| def _wait_for_terminal_cancel(broker: Alpaca, order_id: str, *, timeout: float = 30.0) -> str: | ||
| deadline = time.time() + timeout | ||
| last_status = "" | ||
| while time.time() < deadline: | ||
| raw = broker.api.get_order_by_id(order_id) | ||
| last_status = _status_text(raw) | ||
| if last_status in {"canceled", "cancelled"} or last_status.endswith(".canceled"): | ||
| return last_status | ||
| if last_status in {"filled", "rejected", "expired"}: | ||
| return last_status | ||
| time.sleep(0.25) | ||
| return last_status | ||
|
|
||
|
|
||
| class _LiveOrderDataStrategy(Strategy): | ||
| def initialize(self): | ||
| self.sleeptime = "1S" | ||
| self.vars.iterations = 0 | ||
| self.vars.submitted_order_id = None | ||
| self.vars.stock_price = None | ||
| self.vars.bar_count = 0 | ||
|
|
||
| def on_trading_iteration(self): | ||
| self.vars.iterations += 1 | ||
|
|
||
| asset = Asset("AAPL", asset_type=Asset.AssetType.STOCK) | ||
| price = self.get_last_price(asset) | ||
| assert price is not None and float(price) > 0 | ||
| self.vars.stock_price = float(price) | ||
|
|
||
| bars = self.get_historical_prices(asset, 3, "day") | ||
| assert bars is not None and bars.df is not None and not bars.df.empty | ||
| self.vars.bar_count = len(bars.df) | ||
|
|
||
| # Non-marketable by design: CI must prove real submit/cancel wiring without taking a fill. | ||
| order = self.create_order( | ||
| asset, | ||
| 1, | ||
| Order.OrderSide.BUY, | ||
| order_type=Order.OrderType.LIMIT, | ||
| limit_price=0.01, | ||
| time_in_force="day", | ||
| ) | ||
| submitted = self.submit_order(order) | ||
| assert submitted is not None and submitted.identifier | ||
| self.vars.submitted_order_id = submitted.identifier | ||
| self.broker.cancel_order(submitted) | ||
|
|
||
|
|
||
| class _LiveOptionsChainStrategy(Strategy): | ||
| def initialize(self): | ||
| self.sleeptime = "1S" | ||
| self.options_helper = OptionsHelper(self) | ||
| self.vars.iterations = 0 | ||
| self.vars.call_expirations = 0 | ||
| self.vars.put_expirations = 0 | ||
| self.vars.option_symbol = None | ||
|
|
||
| def on_trading_iteration(self): | ||
| self.vars.iterations += 1 | ||
|
|
||
| underlying = Asset("SPY", asset_type=Asset.AssetType.STOCK) | ||
| underlying_price = self.get_last_price(underlying) | ||
| assert underlying_price is not None and float(underlying_price) > 0 | ||
|
|
||
| chains = self.get_chains(underlying) | ||
| chain_root = chains.get("Chains", {}) if isinstance(chains, dict) else {} | ||
| call_chains = chain_root.get("CALL", {}) | ||
| put_chains = chain_root.get("PUT", {}) | ||
| assert call_chains, "Alpaca returned no SPY call chains" | ||
| assert put_chains, "Alpaca returned no SPY put chains" | ||
| self.vars.call_expirations = len(call_chains) | ||
| self.vars.put_expirations = len(put_chains) | ||
|
|
||
| target_date = datetime.now().astimezone().date() + timedelta(days=7) | ||
| expiry = self.options_helper.get_expiration_on_or_after_date( | ||
| target_date, | ||
| chains, | ||
| "call", | ||
| underlying_asset=underlying, | ||
| ) | ||
| assert expiry is not None | ||
|
|
||
| expiry_key = expiry.strftime("%Y-%m-%d") if hasattr(expiry, "strftime") else str(expiry) | ||
| strikes = call_chains.get(expiry_key) | ||
| assert strikes, f"Alpaca returned no SPY call strikes for {expiry_key}" | ||
| strike = min(strikes, key=lambda value: abs(float(value) - float(underlying_price))) | ||
|
|
||
| option = self.options_helper.find_next_valid_option(underlying, strike, expiry, put_or_call="call") | ||
| assert option is not None | ||
| self.vars.option_symbol = str(option) | ||
|
|
||
|
|
||
| def test_alpaca_run_once_strategy_reads_data_submits_and_cancels_real_order(): | ||
| broker = _require_alpaca() | ||
| strategy = _LiveOrderDataStrategy( | ||
| broker=broker, | ||
| name="alpaca-live-broker-order-ci", | ||
| benchmark_asset=None, | ||
| should_backup_variables_to_database=False, | ||
| should_send_summary_to_discord=False, | ||
| save_logfile=False, | ||
| ) | ||
|
|
||
| try: | ||
| strategy.run_live(run_once=True) | ||
| assert strategy.vars.iterations == 1 | ||
| assert strategy.vars.stock_price > 0 | ||
| assert strategy.vars.bar_count >= 1 | ||
| assert strategy.vars.submitted_order_id | ||
|
|
||
| status = _wait_for_terminal_cancel(broker, strategy.vars.submitted_order_id) | ||
| assert status in {"canceled", "cancelled"} or status.endswith(".canceled") | ||
|
|
||
| all_orders = broker._pull_broker_all_orders() | ||
| assert all_orders is not None | ||
| finally: | ||
| try: | ||
| strategy.cancel_open_orders() | ||
| except Exception: | ||
| pass | ||
| broker.cleanup_streams() | ||
|
|
||
|
|
||
| def test_alpaca_run_once_strategy_reads_options_chain_through_broker_data_source(): | ||
| broker = _require_alpaca() | ||
| strategy = _LiveOptionsChainStrategy( | ||
| broker=broker, | ||
| name="alpaca-live-broker-options-ci", | ||
| benchmark_asset=None, | ||
| should_backup_variables_to_database=False, | ||
| should_send_summary_to_discord=False, | ||
| save_logfile=False, | ||
| ) | ||
|
|
||
| try: | ||
| strategy.run_live(run_once=True) | ||
| assert strategy.vars.iterations == 1 | ||
| assert strategy.vars.call_expirations > 0 | ||
| assert strategy.vars.put_expirations > 0 | ||
| assert strategy.vars.option_symbol | ||
| finally: | ||
| broker.cleanup_streams() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update docs/ and docsrc/ for the new live-broker CI workflow.
This PR adds new live-broker CI test coverage (a new module + workflow) but no accompanying documentation changes are included here. As per coding guidelines, "Every time you work on LumiBot code, you MUST check and update BOTH documentation locations: docs/ ... for AI agents and contributors, docsrc/ ... for end users." Please add engineering notes (docs/) describing this CI workflow/test suite, and update docsrc/ if it affects user-facing testing guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_alpaca_live_broker_apitest.py` around lines 1 - 184, The new
live-broker CI test coverage in the Alpaca apitest module needs matching
documentation updates. Add an engineering note in docs/ describing the new
live-broker CI workflow, what _require_alpaca, _LiveOrderDataStrategy, and
_LiveOptionsChainStrategy validate, and any CI/env requirements. Also update
docsrc/ with any user-facing testing guidance changes so contributors know how
to run or interpret the live-broker tests.
Source: Coding guidelines
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf18606e79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| try: | ||
| strategy.run_live(run_once=True) |
There was a problem hiding this comment.
When this workflow is triggered after hours, on weekends, or on market holidays, StrategyExecutor._run_live_once() returns before calling on_trading_iteration() whenever broker.is_market_open() is false, so this run_live(run_once=True) call leaves strategy.vars.iterations at 0 and the following assertions fail without exercising Alpaca. Because .github/workflows/alpaca-live-broker.yml runs on arbitrary pushes/PRs/manual dispatches, please force a 24/7 test market or otherwise bypass the market-hours gate for these live broker smoke tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/alpaca-live-broker.yml (1)
85-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCredential check omits Alpaca news creds used by the same job.
This step only verifies
ALPACA_TEST_API_KEY/ALPACA_TEST_API_SECRET, but the very next step also runstests/test_agent_alpaca_news_live_apitest.py, which depends onALPACA_NEWS_API_KEY/ALPACA_NEWS_API_SECRET(set at Lines 53-54). If the news secrets are missing, the failure will surface deep inside pytest instead of the clear early::error::message this step is meant to provide.♻️ Proposed fix
- name: Verify Alpaca paper credentials are available run: | set -euo pipefail if [ -z "${ALPACA_TEST_API_KEY}" ] || [ -z "${ALPACA_TEST_API_SECRET}" ]; then echo "::error::ALPACA_TEST_API_KEY and ALPACA_TEST_API_SECRET repository secrets are required for Alpaca live broker CI." exit 1 fi + if [ -z "${ALPACA_NEWS_API_KEY}" ] || [ -z "${ALPACA_NEWS_API_SECRET}" ]; then + echo "::error::ALPACA_NEWS_API_KEY and ALPACA_NEWS_API_SECRET repository secrets are required for the Alpaca news live apitest." + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/alpaca-live-broker.yml around lines 85 - 92, The credential validation step only checks the Alpaca paper secrets, but the same workflow later runs the Alpaca news live test that also needs the news API secrets. Update the verification block in the workflow job to validate both ALPACA_TEST_API_KEY/ALPACA_TEST_API_SECRET and ALPACA_NEWS_API_KEY/ALPACA_NEWS_API_SECRET, and keep the early ::error:: message clear by mentioning all required secrets. Use the existing job step and the downstream test step names to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/alpaca-live-broker.yml:
- Line 60: The job-level timeout on the workflow is shorter than the combined
per-step budgets in the live broker job, so the job can be killed before the
later step timeouts in the run steps ever trigger. Update the job timeout in the
alpaca-live-broker workflow to comfortably exceed the longest possible total
runtime for the steps that use timeout 1500 and timeout 1200, including setup
overhead, so the step-level timeout handling in the live broker
submission/cancellation flow can complete. Refer to the live broker job
definition and its run steps in the workflow to make the adjustment consistently
across the affected section.
---
Outside diff comments:
In @.github/workflows/alpaca-live-broker.yml:
- Around line 85-92: The credential validation step only checks the Alpaca paper
secrets, but the same workflow later runs the Alpaca news live test that also
needs the news API secrets. Update the verification block in the workflow job to
validate both ALPACA_TEST_API_KEY/ALPACA_TEST_API_SECRET and
ALPACA_NEWS_API_KEY/ALPACA_NEWS_API_SECRET, and keep the early ::error:: message
clear by mentioning all required secrets. Use the existing job step and the
downstream test step names to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fe149d3-5f0e-4e21-b0e2-bd6532b5d68e
📒 Files selected for processing (1)
.github/workflows/alpaca-live-broker.yml
mpelteshki
changed the title
This reverts commit fc5e4c7.
mpelteshki
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lumibot/entities/data.py (2)
1244-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTimeshift-to-rows conversion is now triplicated.
The same
timedelta→row conversion logic exists incheck_data.checker(Lines 876-883),_get_bars_dict(Lines 1156-1162), and the new_normalize_timeshift_to_rows. Consider having the first two callself._normalize_timeshift_to_rows(timeshift)to avoid the three copies drifting apart over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lumibot/entities/data.py` around lines 1244 - 1256, The timeshift-to-rows conversion logic is duplicated in `check_data.checker` and `_get_bars_dict`, alongside the new `_normalize_timeshift_to_rows` helper. Update those existing callers to use `self._normalize_timeshift_to_rows(timeshift)` instead of re-implementing the `timedelta` conversion, so the shared logic stays in one place and remains consistent.
1305-1313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralized reliance on private pandas API
_slice().
df_source._slice(slice(start_row, end_row))usesDataFrame._slice, an internal/private pandas method (thepandas.corenamespace is documented as private and not covered by pandas' API stability guarantees). This helper is now the single choke point used by all threeget_bars()fast paths, so any pandas internal change to_slicewould break bar retrieval everywhere at once..iloc[start_row:end_row]is the public equivalent and should have negligible overhead difference.♻️ Suggested fix
- df = df_source._slice(slice(start_row, end_row)) + df = df_source.iloc[start_row:end_row]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lumibot/entities/data.py` around lines 1305 - 1313, Replace the private pandas call in _get_bars_frame_window with the public slicing API, since df_source._slice(slice(start_row, end_row)) is the centralized fast-path used by get_bars() and depends on an unstable internal method. Update the slice operation in _get_bars_frame_window to use the equivalent public indexer on df_source, keeping the same start_row/end_row behavior and return contract so the three fast paths continue to work unchanged.lumibot/tools/databento_helper_polars.py (1)
580-585: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider logging when duplicate timestamps are dropped.
Duplicate
ts_eventrows are silently collapsed (keeping the last) with no log signal. As per coding guidelines,**/{backtesting,data}/**/*.py: "NEVER fabricate, synthesize, or forward-fill missing market data in backtesting. Return empty data, explicit warnings, or skip strategies." Silently dropping duplicate market-data rows without a warning can hide a real data-provider issue; alogger.warning/debugwith the duplicate count would aid diagnosis without changing behavior.📝 Suggested addition
if df.index.has_duplicates: + dup_count = int(df.index.duplicated().sum()) + logger.warning("Dropping %d duplicate timestamp(s) from DataBento frame (keeping last).", dup_count) df = df[~df.index.duplicated(keep="last")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lumibot/tools/databento_helper_polars.py` around lines 580 - 585, The duplicate-timestamp cleanup in the df.index.has_duplicates branch silently drops rows, so add a logger.warning or logger.debug before deduping to report how many duplicate ts_event rows were found and that keep="last" is being applied. Use the existing helper around the index-normalization logic in databento_helper_polars.py so the behavior stays the same while making the data issue visible.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lumibot/entities/data.py`:
- Around line 1239-1313: The native slice fast paths in Data.get_bars() are
bypassing the strict availability checks, so strict_end_check,
_strict_intraday_stale_bar_error, and the usual out-of-range ValueError handling
never run. Update the native branches to invoke the same strict validation used
by the resample path before calling _slice(), likely by reusing
_validate_bars_request() and the row-bound helpers such as
_get_bars_row_bounds()/_get_bars_frame_window(). Keep the fast path, but ensure
strict callers receive the same stale-bar and bounds behavior as non-native
requests.
---
Nitpick comments:
In `@lumibot/entities/data.py`:
- Around line 1244-1256: The timeshift-to-rows conversion logic is duplicated in
`check_data.checker` and `_get_bars_dict`, alongside the new
`_normalize_timeshift_to_rows` helper. Update those existing callers to use
`self._normalize_timeshift_to_rows(timeshift)` instead of re-implementing the
`timedelta` conversion, so the shared logic stays in one place and remains
consistent.
- Around line 1305-1313: Replace the private pandas call in
_get_bars_frame_window with the public slicing API, since
df_source._slice(slice(start_row, end_row)) is the centralized fast-path used by
get_bars() and depends on an unstable internal method. Update the slice
operation in _get_bars_frame_window to use the equivalent public indexer on
df_source, keeping the same start_row/end_row behavior and return contract so
the three fast paths continue to work unchanged.
In `@lumibot/tools/databento_helper_polars.py`:
- Around line 580-585: The duplicate-timestamp cleanup in the
df.index.has_duplicates branch silently drops rows, so add a logger.warning or
logger.debug before deduping to report how many duplicate ts_event rows were
found and that keep="last" is being applied. Use the existing helper around the
index-normalization logic in databento_helper_polars.py so the behavior stays
the same while making the data issue visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36c0cd4a-6fdb-4984-880d-e5f5620922ab
📒 Files selected for processing (15)
.github/workflows/alpaca-live-broker.ymlCHANGELOG.mddocs/ALPACA_LIVE_BROKER_CI.mddocsrc/environment_variables.rstlumibot/backtesting/alpaca_backtesting.pylumibot/entities/data.pylumibot/strategies/strategy_executor.pylumibot/tools/databento_helper.pylumibot/tools/databento_helper_polars.pytests/test_agent_alpaca_news_live_apitest.pytests/test_data_entity.pytests/test_databento_helper.pytests/test_ibkr_futures_daily_series.pytests/test_memory_efficiency_entities_backtesting.pytests/test_strategy_executor_order_index.py
✅ Files skipped from review due to trivial changes (4)
- lumibot/tools/databento_helper.py
- tests/test_strategy_executor_order_index.py
- CHANGELOG.md
- tests/test_ibkr_futures_daily_series.py
Comment on lines
+1239
to
+1313
| @check_data | ||
| def _validate_bars_request(self, dt, length=1, timestep=None, timeshift=0): | ||
| """Run the standard data availability checks without materializing bar data.""" | ||
| return True | ||
|
|
||
| def _normalize_timeshift_to_rows(self, timeshift): | ||
| if timeshift is None: | ||
| return 0 | ||
|
|
||
| if isinstance(timeshift, datetime.timedelta): | ||
| if self.timestep == "day": | ||
| return int(timeshift.total_seconds() / (24 * 3600)) | ||
| if self.timestep == "hour": | ||
| return int(timeshift.total_seconds() / 3600) | ||
| return int(timeshift.total_seconds() / 60) | ||
|
|
||
| return int(timeshift or 0) | ||
|
|
||
| def _get_bars_row_bounds(self, dt, length=1, timeshift=0): | ||
| """Return integer row bounds matching `_get_bars_dict()` slice semantics.""" | ||
| timeshift = self._normalize_timeshift_to_rows(timeshift) | ||
|
|
||
| iter_count = self.get_iter_count(dt) | ||
| try: | ||
| if pd.isna(iter_count): | ||
| iter_count = 0 | ||
| except Exception: | ||
| pass | ||
|
|
||
| # `_get_bars_dict()` slices with `end_row` as an exclusive bound. Daily bars are already | ||
| # complete for intraday requests, so include the as-of daily row. | ||
| if self.timestep == "day": | ||
| end_row = int(iter_count) + 1 - timeshift | ||
| else: | ||
| end_row = int(iter_count) - timeshift | ||
|
|
||
| data_len = getattr(self, "_data_len", None) | ||
| if data_len is None: | ||
| data_len = len(next(iter(self.datalines.values())).dataline) if self.datalines else len(self.df.index) | ||
| self._data_len = int(data_len) | ||
|
|
||
| end_row = max(0, min(end_row, int(data_len))) | ||
| start_row = max(0, end_row - int(length)) | ||
| if start_row > end_row: | ||
| start_row = end_row | ||
| if start_row == end_row and end_row > 0: | ||
| start_row = max(0, end_row - 1) | ||
|
|
||
| return int(start_row), int(end_row), int(timeshift) | ||
|
|
||
| def _get_bars_source_frame(self): | ||
| """Return the cached OHLCV/corporate-action frame used by `get_bars()`.""" | ||
| df_source = getattr(self, "_bars_df", None) | ||
| if df_source is not None: | ||
| return df_source | ||
|
|
||
| try: | ||
| bars_cols = getattr(self, "_bars_cols", None) | ||
| df_source = self.df[bars_cols].copy(deep=False) if bars_cols else self.df | ||
| if bars_cols: | ||
| self._bars_df = df_source | ||
| except Exception: | ||
| df_source = self.df | ||
|
|
||
| return df_source | ||
|
|
||
| def _get_bars_frame_window(self, dt, length=1, timeshift=0): | ||
| """Slice the cached bars frame without building an intermediate dataline DataFrame.""" | ||
| start_row, end_row, normalized_timeshift = self._get_bars_row_bounds(dt, length=length, timeshift=timeshift) | ||
| df_source = self._get_bars_source_frame() | ||
| df = df_source._slice(slice(start_row, end_row)) | ||
| if df is None or df.shape[0] == 0: | ||
| return None, start_row, end_row, normalized_timeshift | ||
| return df, start_row, end_row, normalized_timeshift | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant symbol ranges first.
ast-grep outline lumibot/entities/data.py --view expanded > /tmp/data_outline.txt
printf '%s\n' '--- AST outline (filtered) ---'
sed -n '1,220p' /tmp/data_outline.txt
printf '\n%s\n' '--- Relevant source excerpt around lines 1230-1575 ---'
sed -n '1230,1575p' lumibot/entities/data.py | cat -n
printf '\n%s\n' '--- Search for strict-end/stale-bar logic and fast-path callers ---'
rg -n "_validate_bars_request|strict_end_check|_strict_intraday_stale_bar_error|_get_bars_frame_window|_get_bars_row_bounds|native timestep|native.*1" lumibot/entities/data.py
printf '\n%s\n' '--- Search tests mentioning strict_end_check / stale bars / native minute paths ---'
rg -n "strict_end_check|stale_bar|native.*minute|bucket boundary|_native_timestep_quantity|get_bars" testsRepository: Lumiwealth/lumibot
Length of output: 32889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("lumibot/entities/data.py")
lines = path.read_text().splitlines()
def find_block(start_marker, end_marker=None):
start = next(i for i,l in enumerate(lines,1) if start_marker in l)
if end_marker:
end = next(i for i,l in enumerate(lines[start:], start) if end_marker in l)
else:
end = min(len(lines), start + 220)
return start, end
# Inspect the get_bars method around the native fast paths.
start, end = find_block("def get_bars(", "def get_bars_between_dates(")
segment = "\n".join(f"{i:4d}: {lines[i-1]}" for i in range(start, end+1))
checks = {
"_validate_bars_request calls in get_bars body": len(re.findall(r"_validate_bars_request\(", segment)),
"strict_end_check mentions in get_bars body": len(re.findall(r"strict_end_check", segment)),
"_strict_intraday_stale_bar_error mentions in get_bars body": len(re.findall(r"_strict_intraday_stale_bar_error", segment)),
"_get_bars_frame_window mentions in get_bars body": len(re.findall(r"_get_bars_frame_window", segment)),
}
print("--- get_bars body scan ---")
for k, v in checks.items():
print(f"{k}: {v}")
print("\n--- matching lines near fast paths ---")
for needle in ["_validate_bars_request(", "_strict_intraday_stale_bar_error", "strict_end_check", "_get_bars_frame_window("]:
print(f"\n{needle}:")
for i,l in enumerate(lines[start-1:end], start):
if needle in l:
print(f"{i}: {l}")
print("\n--- tests containing strict_end_check near get_bars calls ---")
for test in ["tests/test_data_entity.py", "tests/test_hour_timestep_support.py"]:
p = Path(test)
if not p.exists():
continue
tlines = p.read_text().splitlines()
for i,l in enumerate(tlines,1):
if "strict_end_check" in l or "get_bars(" in l and "5m" in l:
print(f"{test}:{i}: {l}")
PYRepository: Lumiwealth/lumibot
Length of output: 1438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '700,950p' lumibot/entities/data.py | cat -n
printf '\n%s\n' '--- get_bars method around native fast paths ---'
sed -n '1223,1568p' lumibot/entities/data.py | cat -n
printf '\n%s\n' '--- strict_end_check tests around the cited cases ---'
sed -n '150,340p' tests/test_data_entity.py | cat -nRepository: Lumiwealth/lumibot
Length of output: 41505
Call the strict availability check before the native slice fast paths. Both native branches in Data.get_bars() jump straight to _slice(), so strict_end_check, _strict_intraday_stale_bar_error, and the usual out-of-range ValueError are skipped there. A strict caller can get stale bars back from the fast path instead of the same error/tolerance handling used on the resample path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lumibot/entities/data.py` around lines 1239 - 1313, The native slice fast
paths in Data.get_bars() are bypassing the strict availability checks, so
strict_end_check, _strict_intraday_stale_bar_error, and the usual out-of-range
ValueError handling never run. Update the native branches to invoke the same
strict validation used by the resample path before calling _slice(), likely by
reusing _validate_bars_request() and the row-bound helpers such as
_get_bars_row_bounds()/_get_bars_frame_window(). Keep the fast path, but ensure
strict callers receive the same stale-bar and bounds behavior as non-native
requests.
This was referenced Jul 10, 2026
Collaborator
Author
|
Superseded by #1127, which now contains the useful Alpaca strategy quote, bars, option-chain, and paper-order lifecycle coverage in the required primary CI gate. The duplicate workflow and unrelated branch churn were intentionally not carried over. |
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.
Summary:
Tests: