-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Draft] Try installing Dotnet SDK on Helix Runners for SPMI Benchmark Collections #123723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Draft] Try installing Dotnet SDK on Helix Runners for SPMI Benchmark Collections #123723
Conversation
…ance repo with patch to performance repo dotnet install script
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
|
/azp run runtime-coreclr superpmi-collect-test |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@EgorBo feedback should be addressed! |
EgorBo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This draft PR attempts to move the dotnet SDK installation from the setup phase (superpmi_collect_setup.py) to the execution phase (superpmi_benchmarks.py) for SPMI benchmark collections on Helix runners. The PR aims to set up NuGet cache locations to avoid filling up limited OS disk space on Helix machines.
Changes:
- Removed dotnet SDK installation logic from
superpmi_collect_setup.py - Added NuGet environment variables configuration in
superpmi_benchmarks.py - Moved dotnet SDK installation to occur at runtime in
superpmi_benchmarks.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/scripts/superpmi_collect_setup.py |
Removed the dotnet SDK installation code that used ChangeDir and installed dotnet during setup phase |
src/coreclr/scripts/superpmi_benchmarks.py |
Added NuGet cache environment variables, defined empty install_dotnet_sdk function, and added inline dotnet SDK installation command in build_and_run function |
Comments suppressed due to low confidence (4)
src/coreclr/scripts/superpmi_benchmarks.py:151
- The function
install_dotnet_sdkis defined but has no implementation (empty body). This function appears to be unused in the current code. Consider removing this function definition entirely, or if it was intended to be used in place of the inline dotnet installation code at lines 196-205, then implement it properly and call it frombuild_and_run.
def build_and_run(coreclr_args, output_mch_name):
"""Build the microbenchmarks/real-world and run them under "superpmi collect"
Args:
coreclr_args (CoreClrArguments): Arguments use to drive
output_mch_name (string): Name of output mch file name
"""
src/coreclr/scripts/superpmi_benchmarks.py:205
- The dotnet installation command is missing the
--install-dirparameter. The removed code insuperpmi_collect_setup.py(lines 406-418) explicitly specified--install-dirpointing todotnet_directory. Without this parameter, the dotnet.py script may install to a default location, which would cause the subsequentrun_command([dotnet_exe, "--info"])at line 208 to fail becausedotnet_exeis constructed asos.path.join(dotnet_directory, "dotnet")wheredotnet_directoryisos.path.join(performance_directory, "tools", "dotnet", arch). Add"--install-dir"and thedotnet_directorypath to the command arguments.
"--channels",
"main",
"--verbose"])
# Start with a "dotnet --info" to see what we've got.
run_command([dotnet_exe, "--info"])
tfm = "net11.0"
os.environ["PERFLAB_TARGET_FRAMEWORKS"] = tfm
src/coreclr/scripts/superpmi_benchmarks.py:205
- The call to
make_executable(dotnet_exe)appears to have been removed. On non-Windows platforms, the dotnet executable may need execute permissions to be set after installation. This was previously done before the dotnet installation logic moved fromsuperpmi_collect_setup.pyto this file. Consider addingmake_executable(dotnet_exe)after the dotnet installation at line 205, before attempting to rundotnet --infoat line 208.
src/coreclr/scripts/superpmi_benchmarks.py:202 - Inconsistent indentation: the
archparameter at line 202 has extra indentation compared to the other arguments in the command list. It should be aligned withdotnet_scriptand"install"at the same indentation level as line 199-201.
|
These files aren't participating in the CI build, so I'm going to merge as is. |
|
/ba-g "changes in these files shouldn't trigger any pipeline" |
Requires a patched version of dotnet/performance to set the TLS version in powershell on windows.
dotnet/performance PR here: dotnet/performance#5088