feat: podio +arrow - #389
Open
wdconinc wants to merge 16 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Spack environment configuration to build podio from a pinned git commit and enables the +arrow variant, alongside adding the corresponding upstream spack-packages cherry-pick reference needed to support that variant.
Changes:
- Pin
podioto a specific git SHA inspack-environment/packages.yamland enable+arrow. - Add the
spack-packagescherry-pick commit that introduces thepodioarrowvariant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spack-packages.sh | Adds the upstream cherry-pick hash and description for podio: add arrow variant. |
| spack-environment/packages.yaml | Pins podio to a specific git commit and enables the arrow variant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
wdconinc
enabled auto-merge (squash)
August 13, 2026 15:47
wdconinc
commented
Aug 13, 2026
Contributor
Author
|
Needs: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
spack-environment/packages.yaml:442
- PR description says podio is pinned to git commit 32b44e8fb400cf1919a8a55caadcb6481a52c07d, but this change pins a different commit (808bfbb9e460c3e1c0ef1e2457ae447b67f4af23). Please align either the description or the pinned commit so reviewers/users can reliably reproduce the intended podio version.
- '@git.808bfbb9e460c3e1c0ef1e2457ae447b67f4af23=master'
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
spack-environment/packages.yaml:444
- The PR description says
podiois pinned to commit32b44e8fb400cf1919a8a55caadcb6481a52c07d, but the code pins808bfbb9e460c3e1c0ef1e2457ae447b67f4af23. Please align the code and PR description so reviewers/users know which exact upstream snapshot is being built.
podio:
require:
- '@git.808bfbb9e460c3e1c0ef1e2457ae447b67f4af23=master'
- cxxstd=20
- +arrow +datasource +rntuple
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
spack-environment/packages.yaml:446
- This adds a fork override (package_attributes.git) for podio, but the PR description only mentions pinning a commit from the master branch (without mentioning a forked repo). If the fork is required for +arrow, please call it out in the PR description; otherwise consider removing this override to keep using the upstream podio source configured by the Spack package.
package_attributes:
git: https://github.com/wdconinc/podio.git
spack-environment/packages.yaml:154
- This PR also changes edm4hep from a version constraint ('@1.1') to a specific git SHA while still labeling it as '=1.1'. Since the PR description scopes changes to podio/sio, please either justify this edm4hep pin in the PR description (why tag/URL resolution isn't sufficient) or revert it to avoid an unrelated behavior change.
- '@git.f87de123bb1a56dfab7cdcb7f3cfb9dd51fc9313=1.1'
spack-environment/packages.yaml:442
- PR description says podio is pinned to git commit 32b44e8fb400cf1919a8a55caadcb6481a52c07d, but this requirement pins 4700f4fc497c6e40832e9a0fe574730e4e6acf89 instead. Please align the pinned SHA with the stated intended revision (or update the PR description if 4700f4f… is the correct one).
This issue also appears on line 445 of the same file.
- '@git.4700f4fc497c6e40832e9a0fe574730e4e6acf89=master'
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
.github/workflows/build-push.yml:974
- If eicrecon fails (or the step is cancelled), the script exits immediately and leaves the background npsim process and FIFO behind. That can cause hung jobs or noisy cleanup in the container. Add an EXIT/INT/TERM trap to always remove the FIFO and terminate/wait for npsim.
# Create named pipe for arrow streaming
FIFO_NAME="sim_streaming_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.arrow"
mkfifo "$FIFO_NAME"
spack-environment/packages.yaml:444
- The PR description says podio is pinned to git commit 32b44e8fb400cf1919a8a55caadcb6481a52c07d, but packages.yaml currently pins a different commit (d9068f663c01483eb0b58aa505c088883cfd39e8). This makes it unclear which upstream revision is intended and may lead to building the wrong podio version.
- '@git.d9068f663c01483eb0b58aa505c088883cfd39e8=master'
spack-environment/packages.yaml:140
- This PR description focuses on podio/Arrow changes, but this hunk also switches dd4hep from a released version constraint to a pinned git commit and adds a custom git URL. That’s a materially broader change in package resolution than described; please update the PR description (or split the PR) so reviewers understand dd4hep is now sourced from this git ref/remote.
- '@git.8f0d2193956e0000029e8cad477473a6a2fa31ff=master'
- +ddg4 +ddcad +edm4hep +g4hepem +hepmc3 +xercesc
- any_of: [+ddeve +utilityapps, -ddeve -utilityapps] # FIXME ^root +x +opengl when +utilityapps
package_attributes:
git: https://github.com/wdconinc/DD4hep.git
spack-packages.sh:40
- The PR description says there are two new spack-packages cherry-picks, but this change adds three new cherry-pick SHAs. Please reconcile the PR description with the actual change set (or drop the extra cherry-pick if it’s not intended) to avoid confusion when tracking why spack-packages diverges from the tagged release.
e2a49ec27bbe7a64f1388d296d139754af5f79f4
5d79a44c68046e528f78b3f5e4b42754c1e05c93
818143ef3d6c7f52474e8ff22472247b94ace049
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
spack-environment/packages.yaml:444
- PR description says podio is pinned to commit
32b44e8fb400cf1919a8a55caadcb6481a52c07d, but the code pins podio to@git.d9068f663c01483eb0b58aa505c088883cfd39e8=master. Please align the PR description and the pinned commit (either update the description or change the pinned SHA), so reviewers/users know exactly which podio revision is being consumed.
- '@git.d9068f663c01483eb0b58aa505c088883cfd39e8=master'
.github/workflows/build-push.yml:973
- Because the job runs with
-e, ifeicreconfails the script exits beforewait/rm, leaving the backgroundnpsimprocess running and the FIFO present. Add anEXITtrap that always removes the FIFO and terminatesnpsimto prevent hung/unstable CI jobs.
# Create named pipe for arrow streaming
FIFO_NAME="sim_streaming_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.arrow"
mkfifo "$FIFO_NAME"
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
.github/workflows/build-push.yml:975
- This streaming test can leave the FIFO and the background npsim process behind if eicrecon fails (the step runs with
-e), which can cause hung jobs or noisy cleanup. Add an EXIT trap to always remove the FIFO and terminate the background producer on failure.
# Create named pipe for arrow streaming
FIFO_NAME="sim_streaming_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.arrow"
mkfifo "$FIFO_NAME"
# Start npsim in background, writing to the pipe
spack-environment/packages.yaml:445
- PR description says podio is pinned to commit
32b44e8f..., but the package requirement here pins a different commit (d9068f66...). Please align the PR description with the actual pin (or update the pin if the description is the intended one) so reviewers/users know exactly what podio version is being built.
require:
- '@git.d9068f663c01483eb0b58aa505c088883cfd39e8=master'
- cxxstd=20
spack-packages.sh:40
- The PR description says "two" new spack-packages cherry-picks were added, but this change adds three new commits to the cherry-pick list (including a dd4hep-related commit). Please update the PR description (or drop the extra cherry-pick) so the scope is accurately communicated.
e2a49ec27bbe7a64f1388d296d139754af5f79f4
5d79a44c68046e528f78b3f5e4b42754c1e05c93
818143ef3d6c7f52474e8ff22472247b94ace049
spack-environment/packages.yaml:140
- This PR introduces additional package pins (dd4hep/edm4eic/edm4hep/eicrecon) and also overrides dd4hep's git URL to a fork. None of that scope is mentioned in the PR description, but it can materially change what gets built. Please either document these additional pins/URL override in the PR description or split them into a separate PR for clearer review/auditability.
dd4hep:
require:
- '@git.fd72fb6b0e736e31123941936751ff7100af544d=master'
- +ddg4 +ddcad +edm4hep +g4hepem +hepmc3 +xercesc
- any_of: [+ddeve +utilityapps, -ddeve -utilityapps] # FIXME ^root +x +opengl when +utilityapps
package_attributes:
git: https://github.com/wdconinc/DD4hep.git
Add new GitHub Actions job to test arrow streaming from npsim directly to eicrecon using a named pipe (FIFO). This tests zero-copy streaming between simulation and reconstruction without intermediate file storage. - Job name: npsim-eicrecon-gun-streaming - Tests pi particles with epic_craterlake detector - Runs on both amd64 and arm64 architectures - Uses descriptive FIFO name matching output conventions - Uploads reconstruction artifact for validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
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
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.
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
This pull request updates the
podiopackage configuration and its related package management scripts to add support for the Arrow backend and improve version handling. The most important changes are summarized below:podio package enhancements:
podiopackage is now required at a specific git commit (32b44e8fb400cf1919a8a55caadcb6481a52c07d) from themasterbranch, ensuring a precise version is used.+arrowvariant is enabled forpodio, adding Arrow backend support in addition to the existing+datasourceand+rntuplevariants.spack-packages.sh updates:
podio, and another for improvingurl_for_versionhandling for bothpodioandsio.podio: add arrow variantandpodio,sio: url_for_version for main version.What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply
AI for PR description.