Properly get number of jobs in information panel#11087
Conversation
ric394
added
the
Ready for Software Review
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #11052 by ensuring the “jobs/simulations to run” count shown in the information panel reflects only enabled jobs (excluding disabled simulations).
Changes:
- Added an
Enabledproperty toIRunnableand implemented it across runnable job types. - Updated
SimulationGroupto only add enabled jobs to the job manager (affecting job totals and what gets run). - Added small forwarding/utility enablement properties on some simulation-related types.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTests/APSIMShared/MockJob.cs | Implements new IRunnable.Enabled contract for unit test job stubs. |
| Models/Storage/WriteTableCommand.cs | Implements Enabled for datastore write command jobs. |
| Models/Storage/RevertCheckpointCommand.cs | Implements Enabled for datastore revert command jobs. |
| Models/Storage/EmptyCommand.cs | Implements Enabled for datastore empty command jobs. |
| Models/Storage/DeleteRowsCommand.cs | Implements Enabled for datastore delete-rows command jobs. |
| Models/Storage/DeleteCheckpointCommand.cs | Implements Enabled for datastore delete-checkpoint command jobs. |
| Models/Storage/CleanCommand.cs | Implements Enabled for datastore clean command jobs. |
| Models/Storage/AddCheckpointCommand.cs | Implements Enabled for datastore add-checkpoint command jobs. |
| Models/Core/Simulation.cs | Adds IsEnabled forwarding property (in addition to existing Enabled). |
| Models/Core/Run/SimulationGroup.cs | Filters runnable jobs by job.Enabled before adding to the run queue. |
| Models/Core/Run/SimulationDescription.cs | Exposes Enabled for a simulation description (currently unsafe for null base simulation). |
| Models/Core/Run/EmptyJob.cs | Implements Enabled for the placeholder “no jobs” runnable. |
| APSIM.Shared/JobRunning/JobRunnerSleepJob.cs | Implements Enabled for sleep job. |
| APSIM.Shared/JobRunning/JobManager.cs | Reorders using directives (no functional change). |
| APSIM.Shared/JobRunning/IRunnable.cs | Extends IRunnable with a new Enabled property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary>Returns true if the job is enabled.</summary> | ||
| public bool Enabled => baseSimulation.Enabled; |
There was a problem hiding this comment.
SimulationDescription.Enabled dereferences baseSimulation without a null check, but SimulationDescription is constructed with sim == null in several places (e.g. graph descriptor splitting and unit tests). This will throw NullReferenceException as soon as any caller reads Enabled (including the new filtering logic). Consider returning a safe default when baseSimulation is null (or store an explicit enabled flag on the description).
| public bool Enabled => baseSimulation.Enabled; | |
| public bool Enabled => baseSimulation?.Enabled ?? true; |
| storage.Writer.Clean(names, false); | ||
| } | ||
| foreach (IRunnable job in jobs) | ||
| Add(job); | ||
| if (job.Enabled) | ||
| Add(job); |
There was a problem hiding this comment.
The datastore clean step builds names from all jobs, including those which will now be skipped due to job.Enabled == false. This can delete existing results for disabled simulations which aren't actually going to be run. Consider filtering jobs by Enabled (or checking Enabled when collecting names) so only enabled simulations are cleaned.
| foreach (IRunnable job in jobs) | ||
| Add(job); | ||
| if (job.Enabled) | ||
| Add(job); |
There was a problem hiding this comment.
The new job.Enabled filter changes which simulations/jobs are added to the run. Given this is user-visible (fixes the info panel counter) and affects execution, it would be good to add a unit test covering a disabled simulation/job not contributing to the total and not being executed (e.g. in existing RunnerTests).
| /// <summary> Returns true if the job is enabled</summary> | ||
| bool Enabled { get; } | ||
|
|
There was a problem hiding this comment.
Adding Enabled to the public IRunnable interface is a breaking API change for any external implementers of IRunnable (source/binary). If this interface is used outside this repo, consider a non-breaking alternative (e.g. a new optional interface like IEnableableRunnable, an extension method, or a default interface implementation if the project targets a language/runtime which supports it).
| /// <summary>Is Enabled?</summary> | ||
| public bool IsEnabled => Enabled; | ||
|
|
There was a problem hiding this comment.
Simulation.IsEnabled is a new public property which just forwards to the existing Enabled property from Model. Since it isn’t referenced elsewhere in the codebase, it adds API surface area without changing behaviour; consider removing it and using Enabled directly to avoid confusion/duplication.
| /// <summary>Is Enabled?</summary> | |
| public bool IsEnabled => Enabled; |
|
I think this can be done in a much simpler way. Rather than extend IRunnable, how about instead in SimulationGroup, check if the job is of type Simulation, if it is, cast it to a Simumlation and check the enabled status. If it is not, just add it to the list. That'll make this much cleaner. |
par456
added
Contributor Action Required
resolves #11052
Shows only the actual total number of jobs to be run in the information panel.