Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches the Azure benchmark pipeline from a preinstalled Python toolchain (UsePythonVersion) to using uv for installing Python and the msbench-cli, aiming to speed up dependency installs and support local runs with authenticated private-feed access.
Changes:
- Add an Azure Pipelines step template to install
uvacross OSes. - Update the benchmark pipeline to install Python 3.12 via
uvinstead ofUsePythonVersion. - Update the benchmark runner script to install/run
msbench-cliviauvand add a local-feed-auth fallback usingaz.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
pipelines/templates/steps/install-uv.yml |
Adds cross-platform uv installation steps for pipelines. |
pipelines/scripts/Invoke-CopilotBenchmarks.ps1 |
Switches msbench-cli install/run to uv and adds local feed auth fallback. |
pipelines/azure-benchmarks.yml |
Replaces UsePythonVersion@0 with uv install + uv python install 3.12. |
| python -m pip install msbench-cli --no-input | ||
| uv pip install msbench-cli --no-input | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "pip install msbench-cli failed with exit code $LASTEXITCODE" |
There was a problem hiding this comment.
The failure message still says pip install msbench-cli failed..., but the command is now uv pip install .... This makes debugging harder because logs will reference the wrong tool; update the error text to match the actual command being executed.
| throw "pip install msbench-cli failed with exit code $LASTEXITCODE" | |
| throw "uv pip install msbench-cli failed with exit code $LASTEXITCODE" |
ec75ecf to
72c62db
Compare
| - template: /pipelines/templates/steps/install-uv.yml | ||
|
|
||
| - pwsh: uv python install 3.12 | ||
| displayName: 'Install Python 3.12 with uv' |
There was a problem hiding this comment.
uv python install 3.12 installs a Python runtime, but nothing here ensures subsequent uv pip/uv run commands will actually use 3.12 (and UsePythonVersion@0 was removed). Consider explicitly selecting the interpreter (e.g., passing --python 3.12 to the uv commands or setting the appropriate env var) so the benchmark runs against the intended Python version.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| [string] $VenvName = "venv" | ||
| ) | ||
|
|
||
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve |
There was a problem hiding this comment.
The help text documents a RepoRoot parameter, but the script only accepts VenvName. This mismatch is also what causes callers to pass -RepoRoot and fail. Either add a RepoRoot parameter (and honor it when building $venvPath) or remove the RepoRoot documentation.
| [string] $VenvName = "venv" | |
| ) | |
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve | |
| [string] $VenvName = "venv", | |
| [string] $RepoRoot | |
| ) | |
| if ($PSBoundParameters.ContainsKey('RepoRoot') -and $RepoRoot) { | |
| $repoRoot = $RepoRoot | |
| } | |
| else { | |
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve | |
| } |
| exit 1 | ||
| } | ||
|
|
||
| Write-Host "Activating virtual environment '$VenvName' via VIRTUAL_ENV variable at $venvPath.'" |
There was a problem hiding this comment.
Log message has an extra trailing apostrophe after the venv path (... at $venvPath.'). This looks like a typo and can be confusing when reading pipeline logs.
| Write-Host "Activating virtual environment '$VenvName' via VIRTUAL_ENV variable at $venvPath.'" | |
| Write-Host "Activating virtual environment '$VenvName' via VIRTUAL_ENV variable at $venvPath." |
| if (!(Test-Path $venvPath)) { | ||
| if (Get-Command uv -ErrorAction SilentlyContinue) { | ||
| Write-Host "Creating virtual environment '$VenvName' using uv." | ||
| uv venv $venvPath --verbose | ||
| } | ||
| else { | ||
| $invokingPython = (Get-Command "python").Source | ||
|
|
||
| Write-Host "Creating virtual environment '$VenvName' using virtualenv and python located at '$invokingPython'." | ||
| python -m pip install virtualenv==20.25.1 | ||
| python -m virtualenv $venvPath | ||
| } |
There was a problem hiding this comment.
Native command failures in this script aren’t checked. uv venv, python -m pip install ..., and python -m virtualenv ... can fail without throwing, and the script will continue (PowerShell doesn’t automatically stop on non-zero exit codes from native commands). Consider checking $LASTEXITCODE/$? after each native command and throwing on failure to avoid silently continuing with a missing/broken venv.
| python -m pip install virtualenv==20.25.1 | ||
| python -m virtualenv $venvPath | ||
| } | ||
| $pythonVersion = python --version |
There was a problem hiding this comment.
$pythonVersion = python --version will report the invoking shell’s python, not necessarily the interpreter used to create the venv (especially in the uv venv path). If the intent is to log the venv’s Python version, query the venv interpreter directly (e.g., $venvPath/bin/python or $venvPath/Scripts/python.exe).
| $pythonVersion = python --version | |
| if ($IsWindows) { | |
| $venvPython = Join-Path $venvPath "Scripts/python.exe" | |
| } | |
| else { | |
| $venvPython = Join-Path $venvPath "bin/python" | |
| } | |
| $pythonVersion = & $venvPython --version |
| . "$PSScriptRoot/Create-Venv.ps1" -VenvName "venv" -RepoRoot $repoRoot | ||
| . "$PSScriptRoot/Activate-Venv.ps1" -VenvName "venv" -RepoRoot $repoRoot |
There was a problem hiding this comment.
The dot-sourced venv scripts are invoked with -RepoRoot $repoRoot, but neither Create-Venv.ps1 nor Activate-Venv.ps1 defines a RepoRoot parameter. This will fail with “A parameter cannot be found…” before the benchmark runs. Either add a [string]$RepoRoot param to both scripts (and use it instead of recomputing $repoRoot), or remove -RepoRoot from these calls.
| . "$PSScriptRoot/Create-Venv.ps1" -VenvName "venv" -RepoRoot $repoRoot | |
| . "$PSScriptRoot/Activate-Venv.ps1" -VenvName "venv" -RepoRoot $repoRoot | |
| . "$PSScriptRoot/Create-Venv.ps1" -VenvName "venv" | |
| . "$PSScriptRoot/Activate-Venv.ps1" -VenvName "venv" |
| [string]$VenvName = "venv" | ||
| ) | ||
|
|
||
| Set-StrictMode -Version 4 | ||
| $ErrorActionPreference = "Stop" | ||
|
|
||
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve |
There was a problem hiding this comment.
The help text documents a RepoRoot parameter, but the script only accepts VenvName. Since this script is dot-sourced by the benchmark runner, callers passing -RepoRoot will error. Add a RepoRoot parameter (and use it for $venvPath) or remove the RepoRoot documentation and update callers accordingly.
| [string]$VenvName = "venv" | |
| ) | |
| Set-StrictMode -Version 4 | |
| $ErrorActionPreference = "Stop" | |
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve | |
| [string]$VenvName = "venv", | |
| [string]$RepoRoot | |
| ) | |
| Set-StrictMode -Version 4 | |
| $ErrorActionPreference = "Stop" | |
| if ($RepoRoot) { | |
| $repoRoot = $RepoRoot | |
| } else { | |
| $repoRoot = Join-Path $PSScriptRoot ".." ".." -Resolve | |
| } |
|
|
||
| if (-not (Test-Path $venvPath)) { | ||
| Write-Error "Virtual environment '$venvPath' does not exist at $venvPath" | ||
| exit 1 |
There was a problem hiding this comment.
Because this script is intended to be dot-sourced, calling exit 1 will terminate the entire PowerShell session immediately, which makes it hard for the caller to handle/report errors consistently. Prefer throw here so the calling script can fail naturally (and optionally catch/log) while still stopping execution.
| exit 1 | |
| throw |
|
@copilot, can you iterate on this until it can install and run via UV? |
|
@hallipr Do you still want to merge this PR? |
No description provided.