Skip to content

ref(walltime): select profiler via typed CLI arg#379

Merged
GuillaumeLagrange merged 1 commit into
mainfrom
cod-2750-align-walltime-profiler-selection-with-simulation-tool
May 29, 2026
Merged

ref(walltime): select profiler via typed CLI arg#379
GuillaumeLagrange merged 1 commit into
mainfrom
cod-2750-align-walltime-profiler-selection-with-simulation-tool

Conversation

@GuillaumeLagrange
Copy link
Copy Markdown
Contributor

Align walltime profiler selection with the existing simulation tool pattern.

Previously the walltime profiler override was read by parsing the
CODSPEED_WALLTIME_PROFILER environment variable directly inside
select_profiler, with a manual string match and a warning for unknown
values. This now mirrors how simulation_tool is handled: the override is
exposed as a hidden, value-enum CLI argument (--walltime-profiler, env
CODSPEED_WALLTIME_PROFILER) that clap validates, and is threaded through
the orchestrator config into WallTimeExecutor::new.

The result is consistent argument handling across the codebase and proper
validation of the profiler value (clap rejects unknown values instead of
silently warning and falling back).

The override lives on OrchestratorConfig only, not ExecutorConfig: unlike
simulation_tool (read at run time inside the valgrind executor), the
profiler is selected at executor construction in the orchestrator, where
OrchestratorConfig is in scope — so an ExecutorConfig field would be dead.

Refs COD-2750

Align walltime profiler selection with the simulation tool pattern.
Instead of parsing CODSPEED_WALLTIME_PROFILER directly inside
select_profiler, expose it as a hidden, value-enum CLI argument that
clap validates and thread it through the orchestrator config into
WallTimeExecutor::new. This gives consistent argument handling and
proper validation of the profiler value.

Refs COD-2750
Co-Authored-By: Claude <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR refactors walltime profiler selection from ad-hoc env-var parsing inside select_profiler to a typed clap argument (--walltime-profiler, env CODSPEED_WALLTIME_PROFILER) validated at CLI parse time, mirroring the existing simulation_tool pattern.

  • WalltimeProfiler enum is added to executor/config.rs with ValueEnum, threaded through OrchestratorConfig, and passed into WallTimeExecutor::new so select_profiler no longer reads the environment variable itself.
  • get_executor_from_mode gains a walltime_profiler parameter; the two call sites in the orchestrator correctly pass None for label-building and self.config.walltime_profiler for the actual run.

Confidence Score: 5/5

Safe to merge — this is a clean refactor with no behavioral regression; clap now validates the profiler value instead of silently falling back on unknown input.

All construction sites for OrchestratorConfig and WallTimeExecutor are updated consistently, the label-building call correctly passes None, and the actual run path correctly threads self.config.walltime_profiler through. No pre-existing env-var fallback is silently lost — the new path replicates the same platform-default logic in the None branch.

No files require special attention.

Important Files Changed

Filename Overview
src/executor/wall_time/executor.rs select_profiler refactored to accept an explicit Option instead of reading the env var; logic is cleaner and testable. No issues.
src/executor/config.rs New WalltimeProfiler enum and walltime_profiler field on OrchestratorConfig added cleanly; test() helper updated with None. No issues.
src/executor/mod.rs get_executor_from_mode updated to accept walltime_profiler parameter; get_all_executors passes None. WalltimeProfiler re-exported. Clean.
src/executor/orchestrator.rs Label-building call correctly uses None; actual executor construction correctly passes self.config.walltime_profiler. No issues.
src/cli/shared.rs New hidden walltime_profiler clap field mirrors simulation_tool pattern exactly. No issues.
src/cli/exec/mod.rs walltime_profiler forwarded from shared args to OrchestratorConfig. No issues.
src/cli/run/mod.rs walltime_profiler forwarded to OrchestratorConfig; default struct init sets it to None (appropriate for Simulation mode). No issues.
src/executor/tests.rs Test helpers updated to pass None to WallTimeExecutor::new. No issues.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (shared.rs)
    participant Cfg as OrchestratorConfig
    participant Orch as Orchestrator
    participant Mod as executor/mod.rs
    participant WTE as WallTimeExecutor

    CLI->>Cfg: "walltime_profiler: Option<WalltimeProfiler> (from --walltime-profiler / env)"
    Cfg->>Orch: passed in via build_orchestrator_config()
    Orch->>Mod: get_executor_from_mode(mode, None) [label building only]
    Mod-->>Orch: executor name (profiler arg ignored)
    Orch->>Mod: get_executor_from_mode(mode, self.config.walltime_profiler) [actual run]
    Mod->>WTE: WallTimeExecutor::new(profiler_override)
    WTE->>WTE: select_profiler(profiler_override)
    Note over WTE: Some(Perf) → PerfProfiler<br/>Some(Samply) → SamplyProfiler<br/>None → platform default
Loading

Reviews (1): Last reviewed commit: "ref(walltime): select profiler via typed..." | Re-trigger Greptile

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 29, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 7 untouched benchmarks


Comparing cod-2750-align-walltime-profiler-selection-with-simulation-tool (67223cb) with main (407dd3c)

Open in CodSpeed

@GuillaumeLagrange GuillaumeLagrange merged commit 67223cb into main May 29, 2026
21 checks passed
@GuillaumeLagrange GuillaumeLagrange deleted the cod-2750-align-walltime-profiler-selection-with-simulation-tool branch May 29, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants