Add execution timeouts for simulations#245
Conversation
- 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
[WIP] Add execution timeouts for simulations
[WIP] Add execution timeouts for simulations
[WIP] Add execution timeouts for simulations
There was a problem hiding this comment.
Pull request overview
This pull request introduces execution timeout monitoring for simulations to enhance performance tracking and prevent runaway computations. The changes add wall-clock time limits and no-progress detection through a new ExecutionMonitor component. The PR also updates the run_closed_loop signature to accept execution limits and modifies timestamp recording logic, though this introduces critical bugs. Additionally, the PR simplifies the main simulation flow by removing inline geophysical navigation code from process_file.
Changes:
- Added
ExecutionLimitsandExecutionMonitortypes to enforce wall-clock and no-progress timeouts - Updated
run_closed_loopfunction signature to accept optionalExecutionLimitsparameter - Added CLI arguments for configuring execution timeout behavior
- Modified timestamp tracking logic in
run_closed_loop(introduces type errors) - Updated all test call sites to pass the new
execution_limitsparameter torun_closed_loop
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/sim.rs | Added ExecutionMonitor and ExecutionLimits types with timeout checking logic; modified run_closed_loop to integrate execution monitoring; updated timestamp recording logic (has critical bugs); added test for no-progress timeout |
| core/tests/integration_tests.rs | Updated all run_closed_loop calls to pass None for the new execution_limits parameter |
| sim/src/main.rs | Added CLI arguments for execution limits; integrated execution monitoring in closed-loop and particle filter modes; simplified code by removing inline geophysical navigation (has critical compilation errors in UKF initialization) |
| book/RBPF.md | Updated example code to pass None for new execution_limits parameter |
| .gitignore | Added additional LaTeX artifact patterns |
Comments suppressed due to low confidence (1)
core/src/sim.rs:2038
- The timestamp tracking logic has type errors and will not compile. At line 1947 (not shown in diff),
last_tsis initialized tostart_timewith typeDateTime<Utc>. However, line 2021 comparesSome(ts)withlast_ts(type mismatch), line 2022 attempts to pattern matchlast_tsasSome(prev_ts)(type error), line 2028 assignslast_ts = Some(ts)(type error), and line 2037 assignslast_ts = ts(inconsistent with line 2028). The type oflast_tsneeds to be changed toOption<DateTime<Utc>>and initialized toNoneorSome(start_time). Additionally, the logic needs to be corrected so that states are recorded with the correct timestamps - currently the code would record the filter state after processing event at timetsbut label it withprev_ts.
// If timestamp changed, or it's the last event, record the previous state
if Some(ts) != last_ts {
if let Some(prev_ts) = last_ts {
let mean = filter.get_estimate();
let cov = filter.get_certainty();
results.push(NavigationResult::from((&prev_ts, &mean, &cov)));
debug!("Filter state at {}: {:?}", ts, mean);
}
last_ts = Some(ts);
}
// If this is the last event, also push
if i == total - 1 {
let mean = filter.get_estimate();
let cov = filter.get_certainty();
results.push(NavigationResult::from((&ts, &mean, &cov)));
debug!("Filter state at {}: {:?}", ts, mean);
last_ts = ts;
}
| #[test] | ||
| fn test_execution_monitor_no_progress_timeout() { | ||
| let limits = ExecutionLimits { | ||
| max_wall_clock_ratio: 0.0, | ||
| max_wall_clock_s: 0.0, | ||
| max_no_progress_s: 0.01, | ||
| }; | ||
| let mut monitor = ExecutionMonitor::new(limits, 1.0); | ||
|
|
||
| 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.
The test coverage for ExecutionMonitor only includes the no-progress timeout failure case. Consider adding tests for: (1) wall-clock timeout, (2) successful execution where mark_progress is called regularly and no timeout occurs, and (3) edge cases like zero or negative timeout values.
| let results = match filter_config.filter { | ||
| FilterType::Ukf => { | ||
| let mut ukf = | ||
| initialize_ukf(records[0].clone(), None, None, None, None, None, None); |
There was a problem hiding this comment.
The function call to initialize_ukf has incorrect arguments. The function signature is initialize_ukf(initial_pose: TestDataRecord, config: UkfConfig) but this call passes 7 None arguments. This should be changed to pass UkfConfig::default() as the second argument, or construct a UkfConfig with the desired parameters from filter_config (similar to line 830-837).
| initialize_ukf(records[0].clone(), None, None, None, None, None, None); | |
| initialize_ukf(records[0].clone(), UkfConfig::default()); |
| // Get all CSV files to process | ||
| let csv_files = get_csv_files(&args.sim.input)?; | ||
| let is_multiple = csv_files.len() > 1; | ||
| let _execution_limits = execution_limits_from_args(&args.sim); |
There was a problem hiding this comment.
The _execution_limits variable is created but never used in the run_geo_closed_loop_cli function. If execution limits should be applied to geophysical navigation simulations, this variable should be passed to the appropriate functions. If execution limits are not yet supported for geophysical navigation, consider adding a comment explaining this limitation.
| let _execution_limits = execution_limits_from_args(&args.sim); | |
| // NOTE: Execution limits are not yet applied in geophysical closed-loop simulations. | |
| // We still parse the arguments here to validate them and keep CLI behavior consistent. | |
| let _ = execution_limits_from_args(&args.sim); |
| #[derive(Clone, Debug)] | ||
| pub struct ExecutionMonitor { | ||
| start_time: Instant, | ||
| last_progress: Instant, | ||
| max_wall_clock: Option<StdDuration>, | ||
| max_no_progress: Option<StdDuration>, | ||
| } | ||
|
|
||
| impl ExecutionMonitor { | ||
| pub fn new(limits: ExecutionLimits, sim_duration_s: f64) -> Self { | ||
| let max_wall_clock = compute_max_wall_clock( | ||
| sim_duration_s, | ||
| limits.max_wall_clock_ratio, | ||
| limits.max_wall_clock_s, | ||
| ); | ||
| let max_no_progress = if limits.max_no_progress_s > 0.0 { | ||
| Some(StdDuration::from_secs_f64(limits.max_no_progress_s)) | ||
| } else { | ||
| None | ||
| }; | ||
| let now = Instant::now(); | ||
|
|
||
| Self { | ||
| start_time: now, | ||
| last_progress: now, | ||
| max_wall_clock, | ||
| max_no_progress, | ||
| } | ||
| } | ||
|
|
||
| pub fn check(&mut self, context: &str) -> Result<()> { | ||
| let now = Instant::now(); | ||
| if let Some(max_wall_clock) = self.max_wall_clock | ||
| && 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() | ||
| ); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The ExecutionMonitor struct and its new and check methods lack doc comments. According to the coding guidelines, public APIs should have comprehensive documentation. Consider adding doc comments that explain the purpose of ExecutionMonitor, what the new method does (especially the sim_duration_s parameter), and what the check method validates.
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct ExecutionLimits { | ||
| /// Max wall-clock time as a ratio of simulated duration (<= 0 disables). | ||
| #[serde(default = "default_max_wall_clock_ratio")] | ||
| pub max_wall_clock_ratio: f64, | ||
| /// Max wall-clock time per trajectory in seconds (<= 0 disables). | ||
| #[serde(default = "default_max_wall_clock_s")] | ||
| pub max_wall_clock_s: f64, | ||
| /// Max wall-clock time without progress in seconds (<= 0 disables). | ||
| #[serde(default = "default_max_no_progress_s")] | ||
| pub max_no_progress_s: f64, | ||
| } |
There was a problem hiding this comment.
The ExecutionLimits struct lacks a doc comment. According to the coding guidelines, code should be well-documented with clear and concise comments explaining the purpose of data structures. Consider adding a doc comment that explains the overall purpose of ExecutionLimits and how the different timeout mechanisms work together.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@jbrodovsky I've opened a new pull request, #246, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Add comprehensive documentation for ExecutionLimits struct - Add comprehensive documentation for ExecutionMonitor struct and methods - Fix incorrect arguments to initialize_ukf at line 419 in sim/src/main.rs - Add comment explaining unused execution_limits in geo navigation - Add test coverage for ExecutionMonitor (wall-clock, success case, edge cases) - Fix type mismatch bug for last_ts variable in run_closed_loop - Update integration tests to pass execution_limits parameter Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
…arguments Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
Address PR #245 review: add documentation, fix initialization bug, expand test coverage
Introduce execution timeouts for simulations to enhance performance monitoring. Fix the no-progress timeout bug in the ExecutionMonitor to ensure accurate detection of inactivity. Apply code formatting and linting improvements throughout the codebase.