Repository navigation
[Tools] Run simulations in parallel - #227
Conversation
|
🧱 Stack PR · Base of stack Stack Structure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the simulation batching architecture, moving orchestration from Unity's RunManager to a Python script (Tools/run_batch.py) and introducing a RunWorker component for single-run execution. The update includes support for parallel runs via a new max_parallel configuration field, revised protobuf compilation, and updated documentation. Review feedback identifies an incorrect import path in the batch script and recommends enhancing error handling to ensure that a single worker failure does not terminate the entire batch process.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR replaces Unity-side batch execution (RunManager) with a Python-orchestrated architecture. RunConfig proto gains a Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tools/run_batch.py`:
- Line 14: The import path in run_batch.py is incorrect: replace the failing
`from proto.run_config_pb2 import run_config_pb2` with a proper import that
matches the generated module (e.g., `from pb import run_config_pb2`) or
move/rename the generated file to match `proto.run_config_pb2`; ensure Tools is
on sys.path when running `python3 Tools/run_batch.py` (or `python3
../../Tools/run_batch.py` from Build/<timestamp>) so `pb.run_config_pb2` can be
resolved — update the import in Tools/run_batch.py (the top-level import of
run_config_pb2) accordingly and verify both documented invocation paths succeed.
- Around line 11-14: The module google.protobuf.text_format is not imported so
calls like google.protobuf.text_format.Parse(...) will raise AttributeError; fix
by importing the submodule (e.g., add "from google.protobuf import text_format"
or "import google.protobuf.text_format" at the top alongside the existing
imports) and then either call text_format.Parse(...) or keep the existing
google.protobuf.text_format.Parse(...) after the explicit import so runs of
run_batch.py succeed.
In `@Tools/utils.py`:
- Line 28: The sort in find_all_files (return sorted(files, key=str)) is
redundant because callers like find_all_telemetry_files and find_all_event_logs
apply their own _run_log_sort_key; remove the in-function sorting so
find_all_files simply returns the list of found Path objects in discovery order,
and ensure any caller that requires deterministic ordering (e.g.,
find_latest_file) explicitly sorts or uses max(...) with the appropriate key;
update or confirm find_latest_file and other callers perform their own sorting
or selection logic using _run_log_sort_key where needed.
- Line 10: The regex stored in _RUN_DIRECTORY_PATTERN only captures the run
number and omits the trailing seed digits, so fullmatch fails for names like
"run_1_seed_200"; update _RUN_DIRECTORY_PATTERN to capture the seed digits as
well and anchor the end of the pattern (e.g., add a second capture for the seed
and a terminal anchor) so fullmatch will succeed for complete directory names.
- Around line 51-57: The regex used to detect run directories doesn't include
the seed number so _run_log_sort_key never matches real dirs; update the
_RUN_DIRECTORY_PATTERN definition to use the pattern r"run_(\d+)_seed_\d+"
(i.e., include \d+ after _seed_) so that _run_log_sort_key can match
parent.name, extract the run index via match.group(1), and return the intended
primary sort key instead of falling back to the secondary branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84ab29dd-f5bf-452c-9ded-e24ae7cef940
⛔ Files ignored due to path filters (6)
Assets/Resources/Prefabs/Managers/RunManager.prefabis excluded by!**/*.prefabAssets/Scenes/MainScene.unityis excluded by!**/*.unityAssets/Scripts/Generated/Proto/CommunicationConfig.csis excluded by!**/generated/**Assets/Scripts/Generated/Proto/CommunicationConfig.cs.metais excluded by!**/generated/**Assets/Scripts/Generated/Proto/RunConfig.csis excluded by!**/generated/**Assets/Scripts/Generated/Proto/RunConfig.cs.metais excluded by!**/generated/**
📒 Files selected for processing (29)
.pre-commit-config.yamlAssets/Proto/Configs/run_config.protoAssets/Proto/Scripts/compile.shAssets/Resources/Prefabs/Managers/RunManager.prefab.metaAssets/Scripts/Config/ConfigLoader.csAssets/Scripts/Managers/RunManager.csAssets/Scripts/Managers/RunManager.cs.metaAssets/Scripts/Managers/RunWorker.csAssets/Scripts/Managers/RunWorker.cs.metaAssets/Scripts/Managers/SimManager.csAssets/Scripts/Monitors/SimMonitor.csAssets/Scripts/Plugins/Protobuf.csAssets/Tests/EditMode/RunWorkerTests.csAssets/Tests/EditMode/RunWorkerTests.cs.metaAssets/Tests/EditMode/SanityTests.csAssets/Tests/EditMode/SanityTests.cs.metaAssets/Tests/PlayMode/RunWorkerTests.csAssets/Tests/PlayMode/RunWorkerTests.cs.metaAssets/Tests/PlayMode/SanityTests.csAssets/Tests/PlayMode/SanityTests.cs.metaProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.jsonTools/pb/run_config_pb2.pyTools/run_batch.pyTools/unity_utils.pyTools/utils.pydocs/Simulation_Logging.mddocs/Simulation_Runner.mdplugins/protobuf/protobuf_plugin.ccplugins/protobuf/protobuf_test.cc
💤 Files with no reviewable changes (10)
- Assets/Tests/PlayMode/SanityTests.cs
- Assets/Tests/EditMode/SanityTests.cs
- plugins/protobuf/protobuf_plugin.cc
- Assets/Scripts/Managers/RunManager.cs.meta
- Assets/Tests/PlayMode/SanityTests.cs.meta
- Assets/Tests/EditMode/SanityTests.cs.meta
- Assets/Resources/Prefabs/Managers/RunManager.prefab.meta
- Assets/Scripts/Config/ConfigLoader.cs
- Assets/Scripts/Plugins/Protobuf.cs
- Assets/Scripts/Managers/RunManager.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tools/run_batch.py`:
- Around line 226-245: Failure messages for runs lack the Unity log path; update
the error reporting in the loop that iterates running_workers (the block using
finished_run_index, run_index, descriptor, process) so that both RuntimeError
branches (non-zero exit_code and missing descriptor.output_dir) include the
per-run Unity log path constructed from unity_log_dir, run_index and
descriptor.seed (or descriptor.run_index/seed as appropriate). Build the
deterministic log filepath the same way other code does (use the existing
unity_log_dir variable and the run_index/seed values) and append it to the
f-string messages so users are pointed directly to the Unity log for debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84e75dd3-f4cd-4244-a507-5408c6df1827
⛔ Files ignored due to path filters (4)
Assets/Plugins/assignment.dllis excluded by!**/*.dllAssets/Plugins/libprotobuf.dylibis excluded by!**/*.dylibAssets/Plugins/libprotobuf.sois excluded by!**/*.soAssets/Plugins/protobuf.dllis excluded by!**/*.dll
📒 Files selected for processing (2)
Tools/run_batch.pyTools/utils.py
There was a problem hiding this comment.
More docstrings/comments/documentation here would be useful imo
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/Proto/Scripts/compile.sh`:
- Around line 31-43: The script currently deletes and then invokes protoc into
$PYTHON_OUTPUT_DIR without ensuring the directory exists; update the compile.sh
logic around the Python run config step to create $PYTHON_OUTPUT_DIR if missing
before removing or generating files (use $PYTHON_OUTPUT_DIR as the target), i.e.
add a directory creation guard prior to the rm -f
"$PYTHON_OUTPUT_DIR/run_config_pb2.py" and before the protoc call that generates
run_config_pb2.py so protoc has an existing output directory; keep references to
run_config_pb2.py, PYTHON_OUTPUT_DIR, and the protoc invocation in
Assets/Proto/Scripts/compile.sh.
In `@docs/Simulation_Runner.md`:
- Around line 63-64: Docs show inconsistent CLI flag names and example
invocations; update all occurrences to match the actual script interface by
choosing the canonical flag names used in the code (e.g., use --unity_path or
--binary_path consistently — replace any --binary_path/--binary-path variants
with the actual flag used by the runner, and unify --run-config vs --run_config
to the single canonical form), and fix example commands so the script filename
and working-directory paths match real usage; search for and replace the
inconsistent symbols (--binary_path, --unity_path, --run-config, --run_config
and any mismatched invocation paths) in the doc sections (including the other
mentions around lines noted) so every flag and example matches the implemented
CLI.
In `@Tools/run_batch.py`:
- Around line 135-145: The current _compute_max_parallel treats a proto3-unset
max_parallel (which is 0) as 1 and forces sequential runs; change the logic to
treat 0 as "unset" and substitute a sensible default (e.g., min(16,
run_config.num_runs)) before enforcing bounds; specifically, in
_compute_max_parallel check if run_config.max_parallel == 0 then set
requested_parallelism = min(16, run_config.num_runs) else set
requested_parallelism = max(1, run_config.max_parallel), and finally return
min(requested_parallelism, run_config.num_runs) so omitted proto fields result
in parallel execution instead of sequential.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e2927f0-64a7-4fe6-b311-814587e4a237
⛔ Files ignored due to path filters (1)
Assets/StreamingAssets/Configs/MODULE.bazel.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Assets/Proto/Scripts/compile.shAssets/Scripts/Managers/RunWorker.csAssets/StreamingAssets/Configs/Runs/batch_5_swarms_1000_ucav.pbtxtAssets/StreamingAssets/Configs/Runs/batch_5_swarms_100_ucav.pbtxtAssets/StreamingAssets/Configs/Runs/batch_5_swarms_500_ucav.pbtxtAssets/StreamingAssets/Configs/Runs/batch_7_quadcopters.pbtxtTools/run_batch.pydocs/Simulation_Runner.mdplugins/protobuf/protobuf_test.cc
This PR allows batch simulation runs to be executed in parallel, up to the maximum number of parallel jobs given by the flag
--max_parallel. This PR adds a Python orchestrator inrun_batch.pythat kicks off Unity subprocesses, each with its own simulation configuration, seed, and output directory.Running
batch_7_quadcopters.pbtxtwith 16 simulations in parallel:Example logs: