Skip to content

Add opt-in validation for self-contained xRay inputs - #108

Open
tpn wants to merge 2 commits into
mainfrom
codex/hdf5-input-20261019
Open

tpn wants to merge 2 commits into
mainfrom
codex/hdf5-input-20261019

Conversation

@tpn

@tpn tpn commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

HDF5 inputs can depend on other files through links, external raw storage, or virtual datasets. Add CUPHOTON_XRAY_HDF5_SELF_CONTAINED=1 for xRay workflows that require ordinary datasets stored in the selected file. Existing linked-input behavior remains the default.

The opt-in mode checks links and dataset storage during probing and reading, including reopened handles and visualization. Distributed worker commands retain the setting in saved local and Slurm scripts. Input handles select their policy once, and threaded block readers reuse that selection while validating each reopened handle. This avoids repeated environment checks for every object or block.

Validation: the xRay suite passes in default and opt-in modes (364 passed, 35 GPU-dependent skips each); repository hooks and mypy pass. Local warm-cache benchmarks show about 4 microseconds of remaining normal-probe overhead, with metadata-heavy probes, block reads using a captured policy, and contiguous/gzip trace processing within measurement noise. These measurements do not cover full concurrent GPU throughput. Documentation explains policy lifetime and the validation boundary.

@tpn
tpn requested a review from melo-gonzo as a code owner October 19, 2026 16:40
@tpn tpn added the ai-review Request a focused CodeRabbit review label Oct 19, 2026
@tpn

tpn commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/cuPhoton/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 3d17eb5d-6078-4442-a7e6-7989bb544866
📥 Commits

Reviewing files that changed from the base of the PR and between 4347cd7 and 9a4792f.

📒 Files selected for processing: 7
  • docs/xray/README.md
  • src/cuphoton/xray/_hdf5_input.py
  • src/cuphoton/xray/detector_artifacts.py
  • src/cuphoton/xray/detector_distributed.py
  • src/cuphoton/xray/hdf5.py
  • src/cuphoton/xray/workflow_viz.py
  • tests/xray/test_hdf5_input.py
🚧 Files skipped from review as they are similar to previous changes: 1
  • docs/xray/README.md

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.

📝 Walkthrough

The change adds an opt-in HDF5 input policy that rejects external or soft links, external raw storage, and virtual datasets. xRay workflows use the shared reader, and distributed worker commands preserve the setting.

Changes: HDF5 input validation

Layer / File(s) Summary
Define the HDF5 input policy
src/cuphoton/xray/_hdf5_input.py, docs/xray/README.md, tests/xray/test_hdf5_input.py
The setting accepts only "", "0", or "1". Enabled mode permits in-file hard links and ordinary datasets. Documentation and tests cover policy behavior.
Use validated access in xRay workflows
src/cuphoton/xray/hdf5.py, src/cuphoton/xray/detector_artifacts.py, src/cuphoton/xray/workflow_viz.py, tests/xray/test_hdf5_input.py
HDF5 probing, trace loading, detector artifact generation, normalization-cache creation, block reads, and visualization use the shared reader. Tests cover rejected inputs, rereads, and threaded readers.
Propagate policy to distributed workers
src/cuphoton/xray/detector_distributed.py, tests/xray/test_detector_distributed.py, tests/xray/test_hdf5_input.py
Distributed shape discovery uses the shared reader. Worker commands preserve the enabled setting. Tests cover launcher behavior and generated command argument handling.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 9a479

Saved distributed commands retain the enabled input policy. No confirmed issue blocks merging after normal checks.

Pre-merge checks: Passed 2
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@melo-gonzo melo-gonzo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The opt-in policy checks consumed HDF5 paths and reopened reads while preserving default linked-input behavior. All 75 HDF5/distributed tests passed in both default and opt-in environments, including saved worker commands.

tpn added 2 commits October 19, 2026 21:36
Preserve normal HDF5 linked-data reads by default. Allow workflows to
require ordinary in-file datasets, checking links and storage on each
opened handle and retaining the policy in distributed worker commands.

Signed-off-by: Trent Nelson <trentn@nvidia.com>
Reuse ordinary h5py lookup when validation is disabled and retain the
selected policy across threaded block reads. Validate each newly opened
handle when the strict policy is enabled.

Signed-off-by: Trent Nelson <trentn@nvidia.com>
@tpn
tpn force-pushed the codex/hdf5-input-20261019 branch from 4347cd7 to 9a4792f Compare October 20, 2026 04:38

This branch has not been deployed

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

Labels

ai-review Request a focused CodeRabbit review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants