feat(config): route SEvt output directory through app config#322
Conversation
e0a5711 to
e3432c3
Compare
e3432c3 to
967e5d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 967e5d6a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refactors event output directory selection so that application-level JSON config (via gphox::Config) can drive SEvt output folder placement by synchronizing settings into SEventConfig.
Changes:
- Add
event.output_dirsupport togphox::Configand apply it viaSEventConfig::SetOutFold. - Update
SEvt::DefaultBaseto honor explicitSEventConfig::OutFold()overrides when determining the base output path. - Update example/test executables and scripts to use config-driven behavior (plus bump project C++ standard to C++20).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_simg4ox.sh | Stops forcing fake USER/GEOM, adds set -e, quotes paths. |
| tests/test_GPURaytrace.sh | Removes fake USER/GEOM env-prefix for execution. |
| tests/test_GPUPhotonSource_8x8SiPM.sh | Removes fake USER/GEOM env-prefix for execution. |
| tests/test_GPUPhotonFileSource.sh | Removes fake USER/GEOM env-prefix for execution. |
| tests/compare_ab.py | Adds --base to locate event folders under a configurable root. |
| sysrap/SEvt.cc | Routes DefaultBase() through explicit OutFold overrides when present. |
| src/GPURaytrace.cpp | Adds --config and constructs gphox::Config to apply config. |
| src/GPUPhotonFileSource.cpp | Adds --config and constructs gphox::Config to apply config. |
| src/GPUCerenkov.cpp | Adds --config and constructs gphox::Config to apply config. |
| src/config.h | Introduces EventMode, plus config fields/methods for applying event policy. |
| src/config.cpp | Implements event mode parsing, output dir reading, and Apply() syncing to SEventConfig. |
| config/dev.json | Adds event.output_dir (set to /tmp). |
| CMakeLists.txt | Raises project C++ standard to C++20; sets CUDA standard to 17. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
- Config::Apply() unconditionally overrides SEventConfig::OutFold. wipes any OPTICKS_OUT_FOLD env, bypasses the $TMP/GEOM/$GEOM/$Exec controlled-dir flow, dumps SEvt output in cwd whenever a config lacks event.output_dir.
This is a problem for all 6 binaries.
Commands that break on PR #322 HEAD:
OPTICKS_OUT_FOLD=/data/run42 simg4ox -g foo.gdml -m run.mac # env override silently wiped → cwd
GEOM=mygeom USER=alice simg4ox -g foo.gdml -m run.mac # controlled-dir flow bypassed → cwd
simg4ox -g foo.gdml -m run.mac -c sphere_leak # config has no output_dir → cwd
GPUPhotonSource -g 8x8.gdml -c 8x8SiPM_crystal -m run.mac # same → cwd
cd /home/alice && simg4ox -c wls_test ... # writes A000/B000/ into $HOME
- storch torch left uninitialized when JSON omits the torch section, primitives are indeterminate, generate_photons(cfg.torch) reads UB. Commented on specific lines.
Command that breaks:
GPHOX_CONFIG_DIR=/tmp simg4ox -c notorch -g foo.gdml -m run.mac # generate_photons reads uninit struct
- Generic catch (std::exception&) too broad, it swallows filesystem and any non-JSON throws under "Failed reading config parameters."
Some more non-blockers:
- gphox::EventMode duplicates SEventConfig's mode strings. src/config.h:13-23 mirrors sysrap/SEventConfig.hh:387-394. Drift risk if sysrap adds a mode. Suggest driving EventModeInfos at src/config.cpp:37-46 off SEventConfig::Minimal etc. directly.
- config/dev.json:28 ships "output_dir": "/tmp": concurrent users on one host collide at /tmp/ALL0_no_opticks_event_name/.
Questions:
- Is the OPTICKS_OUT_FOLD env-var override regression intentional?
- Why is dev.json shipping a host-wide /tmp instead of a per-user path?
That is exactly the motivation behind this PR — rely only on the config file to control the output dir and other runtime parameter and dont' use env vars |
yes, all parameters should be defined via json configs
Agree and switched to the current directory by default. The user can set it to any path via a config file. |
changed to |
1466af0 to
c1a0a63
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1a0a630e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The direction (config-only over env vars) is a good one, but only if all env variables eventually move to JSON; otherwise developers have to keep two mental models (env + JSON) to predict a run, which is worse than either extreme. Reproducibility-affecting knobs like |
The plan is to migrate the rest gradually over time. You are welcome to help of course. |
This branch refactors output directory handling so app-level JSON config can control where event folders are written, instead of relying only on default `SEventConfig` behavior/env vars. - Centralize runtime output path control in the application config layer. - Make output location explicit and reproducible across binaries. - Reduce dependence on ad-hoc env setup in test and local runs. If `event.output_dir` is set, SEvt output folders are rooted there.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
c1a0a63 to
c3a2829
Compare
|
@ggalgoczi Do you have any other questions? |
This PR makes
gphox::Configthe authoritative source for runtime configuration parameters that are applied to supported binaries execution. The main focus is refactoring event runtime controls, especiallyevent.modeandevent.output_dir, so they are read from the app config JSON and then synchronized intoSEventConfigfrom one place.--configsupport to the affected binaries so runtime behavior can be selected via config file.USER/GEOMenvironment setup.USERandGEOMenvironment variables for the supported binaries/tests that now route output throughConfig.