Offset event_id across multi-shard reference reads in giant analyze (gitea #22) #69

Merged

lars merged 1 commits from fix/issue-22 into master 2026-08-18 10:23:21 +02:00

Conversation 0 Commits 1 Files Changed 2

+67 -3

lars commented 2026-08-17 15:54:16 +02:00

Owner

Copy Link

Copy Source

giant/analysis/sources.py's open_side scanned a reference directory of
parquet shards with a bare glob and never offset event_id across them.
Each shard is a separate Geant4 job whose own event_id numbering restarts
from 0, so events from different shards collided on the same event_id,
corrupting every downstream per-event grouping and the event_id % n_chunks
condor chunking — the same root cause already fixed on the training/rollout
side via giant/data/loader.py's per-file event_id_offset.

open_side's reference branch now uses find_parquet_files (the same
deterministically ordered file lister giant rollout's _seed_from_data uses)
and offsets each shard's event_id via a join on polars' include_file_paths,
so both sides of a comparison agree on what an event_id means. Two
incidental behaviour changes come along for free: .manifest references now
work (they crashed before), and the directory glob narrows from recursive
**/*.parquet to top-level *.parquet, matching the file list rollout itself
used to assign offsets — a deliberate choice, since a differing file list
would make the two sides' offsets disagree again in a subtler way.

No overflow guard on the per-shard offset stride (unlike loader's
_offset_event_id): checking it here would cost an eager event_id-column
read per shard in every condor compute job, and giant rollout already runs
that check over the same file list when producing the seed.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

giant/analysis/sources.py's open_side scanned a reference directory of parquet shards with a bare glob and never offset event_id across them. Each shard is a separate Geant4 job whose own event_id numbering restarts from 0, so events from different shards collided on the same event_id, corrupting every downstream per-event grouping and the event_id % n_chunks condor chunking — the same root cause already fixed on the training/rollout side via giant/data/loader.py's per-file event_id_offset. open_side's reference branch now uses find_parquet_files (the same deterministically ordered file lister giant rollout's _seed_from_data uses) and offsets each shard's event_id via a join on polars' include_file_paths, so both sides of a comparison agree on what an event_id means. Two incidental behaviour changes come along for free: .manifest references now work (they crashed before), and the directory glob narrows from recursive **/*.parquet to top-level *.parquet, matching the file list rollout itself used to assign offsets — a deliberate choice, since a differing file list would make the two sides' offsets disagree again in a subtler way. No overflow guard on the per-shard offset stride (unlike loader's _offset_event_id): checking it here would cost an eager event_id-column read per shard in every condor compute job, and giant rollout already runs that check over the same file list when producing the seed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

lars added 1 commit 2026-08-17 15:54:17 +02:00

Offset event_id across multi-shard reference reads in giant analyze (gitea #22) ... c1e6ffd8c6

giant/analysis/sources.py's open_side scanned a reference directory of
parquet shards with a bare glob and never offset event_id across them.
Each shard is a separate Geant4 job whose own event_id numbering restarts
from 0, so events from different shards collided on the same event_id,
corrupting every downstream per-event grouping and the event_id % n_chunks
condor chunking — the same root cause already fixed on the training/rollout
side via giant/data/loader.py's per-file event_id_offset.

open_side's reference branch now uses find_parquet_files (the same
deterministically ordered file lister giant rollout's _seed_from_data uses)
and offsets each shard's event_id via a join on polars' include_file_paths,
so both sides of a comparison agree on what an event_id means. Two
incidental behaviour changes come along for free: .manifest references now
work (they crashed before), and the directory glob narrows from recursive
**/*.parquet to top-level *.parquet, matching the file list rollout itself
used to assign offsets — a deliberate choice, since a differing file list
would make the two sides' offsets disagree again in a subtler way.

No overflow guard on the per-shard offset stride (unlike loader's
_offset_event_id): checking it here would cost an eager event_id-column
read per shard in every condor compute job, and giant rollout already runs
that check over the same file list when producing the seed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

lars merged commit de805fb0a7 into master 2026-08-18 10:23:21 +02:00

lars deleted branch fix/issue-22 2026-08-18 10:23:23 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

architecture

bug

Silent/wrong behavior, no crash

experiment

feature

New feature matching the v0.3 redesign

investigation

modularity

Making the network genuinely pluggable

performance

phase2

validation

wontfix

Not being fixed right now

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

lars (Lars Bogner)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lars/giant#69