Skip to content

Fix run-pass telemetry decorator placement - #2649

Open
Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 2 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/run-pass-action-decorator
Open

Fix run-pass telemetry decorator placement#2649
Sylvester Kaczmarek (sylvesterkaczmarek) wants to merge 2 commits into
microsoft:mainfrom
sylvesterkaczmarek:fix/run-pass-action-decorator

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

Describe your changes

Addresses the run-pass decorator issue identified in #2516.

RunPassCommand currently applies @action to the class itself, while the other CLI commands apply it to their run() methods. The action decorator wraps callables and therefore replaces the class binding with a wrapper function, recording construction rather than the actual command execution.

This change keeps RunPassCommand as a class and applies @action to run() consistently with the rest of the CLI.

Tests

Adds regression coverage verifying that RunPassCommand remains a class and that its run() method is wrapped by the telemetry decorator.

Copilot AI lite review requested due to automatic review settings September 3, 2026 18:33
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change aligns run-pass with existing CLI telemetry patterns and includes a focused regression test validating the intended decorator placement.

Pull request overview

This PR fixes Olive CLI telemetry for the run-pass command by moving the @action decorator from the RunPassCommand class (which unintentionally wraps the class into a function) onto the run() method, aligning it with the established pattern used by other CLI commands.

Changes:

  • Removed @action from RunPassCommand’s class definition so the argparse binding remains a class (not a wrapper function).
  • Added @action to RunPassCommand.run() so telemetry records actual command execution.
  • Added a regression test to ensure RunPassCommand remains a class and that run() is wrapped (via functools.wraps / __wrapped__).
File summaries
File Description
olive/cli/run_pass.py Moves @action from the command class to the run() method to correctly capture execution telemetry and preserve class binding.
test/cli/test_run_pass_action.py Adds regression coverage asserting the command is still a class and the run() method is telemetry-wrapped.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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