Add FXMacroData macro provider#1111
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds an ChangesFXMacroData feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.6)lumibot/macro/fxmacrodata.py************* Module pylintrc ... [truncated 734 characters] ... age-id": "E1102" tests/test_fxmacrodata_macro.py************* Module pylintrc ... [truncated 2799 characters] ... do_not_require_api_key", 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: 4
🧹 Nitpick comments (1)
lumibot/macro/fxmacrodata.py (1)
167-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-atomic cache write risks a corrupted cache file.
If the process is interrupted mid-write,
cache_path.write_text(...)can leave a partial/corrupt JSON file; every subsequent_get_jsoncall for that path would then raise onjson.loads. The codebase already has an established atomic-write pattern (tmp file +os.replace) used elsewhere for exactly this reason (see_strategy.py's_backup_variables_to_scheduled_state_file).♻️ Proposed atomic write
cache_path.parent.mkdir(parents=True, exist_ok=True) - cache_path.write_text(json.dumps(payload, indent=2, sort_keys=True), encoding="utf-8") + tmp_path = cache_path.with_name(f"{cache_path.name}.{os.getpid()}.tmp") + tmp_path.write_text(json.dumps(payload, indent=2, sort_keys=True), encoding="utf-8") + os.replace(tmp_path, cache_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/macro/fxmacrodata.py` around lines 167 - 168, The cache write in the fx macro data path is non-atomic and can leave a partially written JSON file if interrupted. Update the cache persistence logic around the write in the relevant fxmacrodata helper to follow the same tmp-file-plus-os.replace pattern used by _backup_variables_to_scheduled_state_file in _strategy.py. Keep the existing payload serialization, but write to a temporary file first, flush it, then atomically replace cache_path so _get_json never reads a corrupt cache.
🤖 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 `@docs/FRED_MACRO_DATA.md`:
- Around line 56-89: The API-key note in the macro data docs is too broad and
incorrectly implies FXMacroData may require FRED credentials. Update the wording
around the macro tools section so the FRED key requirement applies only to the
FRED-specific built-ins, while keeping the FXMacroData section clear that USD
data is public and non-USD access uses FXMD_API_KEY or FXMACRODATA_API_KEY. Use
the existing macro tool names in the docs, such as list_fxmacrodata_indicators
and get_fxmacrodata_latest, to keep the scope distinction explicit.
In `@docsrc/macro_data.rst`:
- Around line 63-93: The macro data docs currently overstate the API-key
requirement by implying it applies broadly; scope the key note to FRED-only
tools so readers do not think FXMacroData needs FRED credentials. Update the
wording in the macro documentation around the existing FRED mention so it
clearly applies only to the FRED-backed built-ins, while keeping the FXMacroData
section consistent with its public USD access and separate `FXMD_API_KEY` /
`FXMACRODATA_API_KEY` behavior.
In `@lumibot/macro/fxmacrodata.py`:
- Around line 296-320: Rows without a parseable date are currently slipping
through the point-in-time filter in the observation normalization loop. Update
the logic in the row-processing block that builds normalized observations so
that when both announcement_dt and _parse_dt(row_date) are None, the row is
excluded instead of being appended. Keep the existing gating behavior in place
for parsed dates, and make sure the fix is applied where comparison_dt is
computed before observations.append is called.
- Around line 155-169: The FXMacroData cache is being used unconditionally in
`_get_json`, which can cause live runs to reuse stale same-day responses;
restrict the disk cache to backtest-only flows by conditioning the cache
read/write on the existing backtest indicator used in this module. Also tighten
`_normalize_observations` so rows without both `announcement_datetime` and the
fallback date fields are dropped instead of being emitted as
`point_in_time_safe`, preventing malformed API rows from bypassing the PIT gate.
---
Nitpick comments:
In `@lumibot/macro/fxmacrodata.py`:
- Around line 167-168: The cache write in the fx macro data path is non-atomic
and can leave a partially written JSON file if interrupted. Update the cache
persistence logic around the write in the relevant fxmacrodata helper to follow
the same tmp-file-plus-os.replace pattern used by
_backup_variables_to_scheduled_state_file in _strategy.py. Keep the existing
payload serialization, but write to a temporary file first, flush it, then
atomically replace cache_path so _get_json never reads a corrupt cache.
🪄 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: 07ee2e9b-18cb-4d6c-8d18-0c0c6598d563
📒 Files selected for processing (17)
docs/AI_AGENT_BUILTIN_TOOLS.mddocs/AI_TRADING_AGENTS.mddocs/ENV_VARS.mddocs/FRED_MACRO_DATA.mddocsrc/agents.rstdocsrc/agents_builtin_tools.rstdocsrc/environment_variables.rstdocsrc/macro_data.rstlumibot/components/agents/builtins.pylumibot/components/agents/manager.pylumibot/components/agents/runtime.pylumibot/macro/__init__.pylumibot/macro/fxmacrodata.pylumibot/macro/macro_data.pylumibot/strategies/_strategy.pytests/test_agent_tool_permissions.pytests/test_fxmacrodata_macro.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_fxmacrodata_macro.py (1)
73-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated env-var/monkeypatch/FXMacroData setup into a fixture.
The pattern of clearing
FXMD_API_KEY/FXMACRODATA_API_KEY, patchingrequests.get, and constructingFXMacroData(..., min_request_interval_seconds=0)is duplicated verbatim across multiple tests in this file. Consolidating into a pytest fixture (parametrized by the strategy/payload generator) would reduce boilerplate and make future changes (e.g., adding a new required env var) a one-line fixture edit instead of a multi-site update.♻️ Example fixture sketch
+@pytest.fixture +def fxmd_env(monkeypatch): + monkeypatch.delenv("FXMD_API_KEY", raising=False) + monkeypatch.delenv("FXMACRODATA_API_KEY", raising=False) + def _make(strategy, tmp_path, fake_get): + monkeypatch.setattr("lumibot.macro.fxmacrodata.requests.get", fake_get) + return FXMacroData(strategy, cache_dir=tmp_path, min_request_interval_seconds=0) + return _makeAlso applies to: 95-102, 116-123
🤖 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_fxmacrodata_macro.py` around lines 73 - 82, The FXMacroData test setup is duplicated across multiple tests, including clearing FXMD_API_KEY and FXMACRODATA_API_KEY, patching requests.get, and creating FXMacroData with min_request_interval_seconds=0. Extract that repeated initialization into a pytest fixture in test_fxmacrodata_macro.py, and have tests like test_fxmacrodata_drops_rows_without_parseable_dates use the shared fixture while only varying the payload/strategy inputs. This will centralize the environment and network mocking around FXMacroData and make future changes easier.
🤖 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.
Nitpick comments:
In `@tests/test_fxmacrodata_macro.py`:
- Around line 73-82: The FXMacroData test setup is duplicated across multiple
tests, including clearing FXMD_API_KEY and FXMACRODATA_API_KEY, patching
requests.get, and creating FXMacroData with min_request_interval_seconds=0.
Extract that repeated initialization into a pytest fixture in
test_fxmacrodata_macro.py, and have tests like
test_fxmacrodata_drops_rows_without_parseable_dates use the shared fixture while
only varying the payload/strategy inputs. This will centralize the environment
and network mocking around FXMacroData and make future changes easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cc8630d-6a62-4286-a721-e24c5bd62c2e
📒 Files selected for processing (4)
docs/FRED_MACRO_DATA.mddocsrc/macro_data.rstlumibot/macro/fxmacrodata.pytests/test_fxmacrodata_macro.py
✅ Files skipped from review due to trivial changes (2)
- docsrc/macro_data.rst
- docs/FRED_MACRO_DATA.md
1 participant
Summary
self.macroFRED API through aMacroDatacontainer while addingself.macro.fxmacrodata/self.macro.fxmdFXMD_API_KEY,FXMACRODATA_API_KEY, cache/base-url env vars, andX-API-Keyheader transportValidation
python -m pytest tests/test_fxmacrodata_macro.py tests/test_fred_macro.py tests/test_agent_tool_permissions.py -qpython -m ruff check lumibot/macro/fxmacrodata.py lumibot/macro/macro_data.py tests/test_fxmacrodata_macro.pygit diff --checkSummary by CodeRabbit
MacroDataaccess.