Skip to content

fix(etfs): rebuild combined file from partitions, not stale aggregate - #46

Open
mikesdelaney-cmyk wants to merge 1 commit into
ml4t:mainfrom
mikesdelaney-cmyk:fix/regenerate-combined-reads-stale-aggregate
Open

fix(etfs): rebuild combined file from partitions, not stale aggregate#46
mikesdelaney-cmyk wants to merge 1 commit into
ml4t:mainfrom
mikesdelaney-cmyk:fix/regenerate-combined-reads-stale-aggregate

Conversation

@mikesdelaney-cmyk

Copy link
Copy Markdown

Summary

Fixes ETFDataManager._regenerate_combined() so it actually rebuilds etf_universe.parquet from the individual ticker partitions.

Problem

_regenerate_combined() currently calls:

all_data = self.load_all()

but load_all() prefers the existing etf_universe.parquet when that file exists. This means _regenerate_combined() can read the stale combined file and write it straight back out, even after the individual ticker partitions have been updated successfully.

In practice, I reproduced this with the ML4T Chapter 25 ETF deployment loop:

individual ticker partitions advanced through 2026-08-20
ETFDataManager.update() reported new rows
_regenerate_combined() ran
the combined aggregate still ended at 2025-12-31

Deleting the combined file caused load_all() to fall back to load_symbols(...), after which the aggregate rebuilt correctly. That isolated the issue to the cache-preferring behavior of load_all().

Fix

Change _regenerate_combined() to load the configured ticker partitions directly:

all_data = self.load_symbols(self.config.get_all_symbols())

This matches the method docstring (“Regenerate combined file from individual ticker files”) and uses the same partition-loading path that load_all() already falls back to when the aggregate is absent.

Tests

Added regression coverage for:

rebuilding when an existing aggregate is stale
restoring symbols present in ticker partitions but missing from the aggregate

Targeted test run:

37 passed, 2 warnings
End-to-end verification

I installed this branch into the machine-learning-for-trading project via a SHA-pinned uv Git source and reran the Chapter 25 ETF deployment loop with the existing frozen etf_universe.parquet still present.

Before the fix, the combined file remained at:

470,662 rows, ending 2025-12-31

After the fix, the normal refresh path rebuilt it successfully without deleting the aggregate first:

ml4t-data update: 318 new rows across 2 symbols
Prices: 486,552 rows x 100 symbols, 2006-01-03 to 2026-08-20

No changes were made to the deployment logic itself; this PR only fixes aggregate regeneration inside ml4t-data.

@stefan-jansen stefan-jansen added compatibility: none No known compatibility impact priority: high Material user impact requiring prompt remediation status: accepted Accepted for implementation type: bug Confirmed or reported incorrect behavior labels Aug 25, 2026
@stefan-jansen

Copy link
Copy Markdown
Contributor

Accepted as the issue record under the ecosystem external-contribution policy.

I reproduced the defect through the public ETFDataManager.update() workflow on current main: the SPY partition advanced to 2026-08-20 while load_all() still returned the stale aggregate ending 2025-12-31. The same reproducer against commit ce0e7e8 rebuilt the aggregate through 2026-08-20.

Focused verification on the submitted commit passes: 37 ETF/regression tests, Ruff check and format, and ty. Full repository qualification is now running before review approval.

@stefan-jansen stefan-jansen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at ce0e7e8. Reproduced through ETFDataManager.update(): main advanced the partition but left the combined dataset stale, while this head advances both. Full repository qualification passed: Ruff, formatting, ty, 3,611 tests, wheel and sdist installation verification, strict docs build, dependency audit, and pre-commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compatibility: none No known compatibility impact priority: high Material user impact requiring prompt remediation status: accepted Accepted for implementation type: bug Confirmed or reported incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants