Add execution timeouts for simulations#235
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds execution timeout capabilities to simulation runs to prevent runaway computations. The feature introduces configurable wall-clock time limits (both as a ratio of simulation duration and as absolute time) and no-progress timeouts.
Changes:
- Added
ExecutionLimitsandExecutionMonitortypes to core simulation module - Added CLI arguments for timeout configuration with sensible defaults
- Integrated execution monitoring into closed-loop filter and particle filter processing loops
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/sim.rs | Added execution timeout infrastructure including ExecutionLimits struct, ExecutionMonitor with timeout checking logic, and integration into run_closed_loop function |
| core/tests/integration_tests.rs | Updated all run_closed_loop call sites to pass None for the new execution_limits parameter |
| sim/src/main.rs | Added CLI arguments for timeout configuration, integrated execution limits into closed-loop and particle filter modes |
| book/RBPF.md | Updated example code to include new execution_limits parameter |
Comments suppressed due to low confidence (2)
sim/src/main.rs:1344
- Missing initialization of
execution_limitsvariable. The variable is used at line 1491 but never defined in this function. Addlet execution_limits = execution_limits_from_args(&args.sim);after line 1344, similar to how it's done inrun_closed_loop_cli(line 941) andrun_geo_closed_loop_cli(line 1110).
fn run_particle_filter(args: &ParticleFilterSimArgs) -> Result<(), Box<dyn Error>> {
validate_input_path(&args.sim.input)?;
validate_output_path(&args.sim.output)?;
if !matches!(args.filter_type, ParticleFilterType::RaoBlackwellized) {
return Err("Only Rao-Blackwellized particle filter is implemented in this mode".into());
}
let csv_files = get_csv_files(&args.sim.input)?;
let is_multiple = csv_files.len() > 1;
sim/src/main.rs:1117
- The
execution_limitsvariable is extracted from args but never used in the geophysical navigation code paths. Thegeo_closed_loop_ukfandgeo_closed_loop_ekffunctions (lines 1235, 1292) don't accept execution limits parameters, leaving geophysical navigation without timeout protection. Consider adding execution limit parameters to these functions or usingExecutionMonitordirectly within the geonav crate.
let execution_limits = execution_limits_from_args(&args.sim);
if is_multiple {
info!("Processing {} CSV files from directory", csv_files.len());
}
// Process each CSV file
for input_file in &csv_files {
| pub fn check(&mut self, context: &str) -> Result<()> { | ||
| let now = Instant::now(); | ||
| if let Some(max_wall_clock) = self.max_wall_clock { | ||
| if now.duration_since(self.start_time) > max_wall_clock { | ||
| bail!( | ||
| "Execution timeout ({context}): exceeded wall-clock limit of {:.2} s", | ||
| max_wall_clock.as_secs_f64() | ||
| ); | ||
| } | ||
| } | ||
| if let Some(max_no_progress) = self.max_no_progress { | ||
| let since_progress = now.duration_since(self.last_progress); | ||
| if since_progress > max_no_progress { | ||
| bail!( | ||
| "Execution timeout ({context}): no progress for {:.2} s (limit {:.2} s)", | ||
| since_progress.as_secs_f64(), | ||
| max_no_progress.as_secs_f64() | ||
| ); | ||
| } | ||
| } | ||
| self.last_progress = now; |
There was a problem hiding this comment.
The check method always updates last_progress on line 2639, which means the no-progress timeout can never trigger. The last_progress field should only be updated when actual progress is made (e.g., when an event is successfully processed), not on every check. Consider removing line 2639 and requiring callers to explicitly call a separate method like mark_progress() when progress occurs.
| std::thread::sleep(std::time::Duration::from_millis(20)); | ||
| let result = monitor.check("test"); | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
This test will always pass regardless of the no-progress logic due to the bug in Comment 3. The test expects a timeout after 20ms of no progress, but since check() always updates last_progress, the timeout condition can never be satisfied. This test needs to be updated once the no-progress logic is fixed.
| std::thread::sleep(std::time::Duration::from_millis(20)); | |
| let result = monitor.check("test"); | |
| assert!(result.is_err()); | |
| // NOTE: As of now, ExecutionMonitor::check() always updates `last_progress`, | |
| // so the no-progress timeout condition cannot be triggered. This test currently | |
| // verifies that no error is returned in that situation. | |
| // | |
| // TODO: Once the no-progress logic in ExecutionMonitor is fixed so that | |
| // `last_progress` is only updated on real progress, update this test to | |
| // expect a timeout error instead. | |
| std::thread::sleep(std::time::Duration::from_millis(20)); | |
| let result = monitor.check("test"); | |
| assert!(result.is_ok()); |
|
Please format and lint this PR, then conduct a re-review. |
|
@jbrodovsky I've opened a new pull request, #236, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Fix duplicate ExecutionMonitor import in core/src/sim.rs - Apply clippy suggestions to collapse nested if statements - Fix mutable borrow issues in particle filter code - Apply rustfmt formatting throughout the workspace - Add execution_limits to run_particle_filter function Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
- Remove automatic last_progress update from check() method - Add mark_progress() method to explicitly mark progress - Update all callers to call mark_progress() after successful event processing - Fix test_execution_monitor_no_progress_timeout to properly test the timeout The no-progress timeout can now correctly detect when no progress has been made for the specified duration. Previously, check() was updating last_progress on every call, which prevented the timeout from ever triggering. Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
Fix no-progress timeout bug and apply code formatting
|
@copilot |
|
@jbrodovsky I've opened a new pull request, #241, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] Add execution timeouts for simulations
|
@copilot |
|
@jbrodovsky I've opened a new pull request, #243, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] Add execution timeouts for simulations
[WIP] Add execution timeouts for simulations
|
Closed due to merge conflict |
Summary
Testing