-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Toolset update: VS 2022 17.10 Preview 4, non-spot VMs, Azure Pipelines overhaul #4594
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StephanTLavavej
added
infrastructure
Related to repository automation
uncharted
Excluded from the Status Chart
labels
Apr 16, 2024
This matches how it's being passed to cmake-configure-build.yml and run-tests.yml.
…argetArch`. This matches what's being passed to cmake-configure-build.yml and run-tests.yml.
…Run Tests'. Spending logic to vary the `displayName` is unnecessary, especially because we already distinguish 'Build and Test x64' vs. 'Build ARM64' at the top level.
This clarifies who the ultimate consumer is.
…th a default argument. This is more complicated, but it's a step towards unification.
…an'` into a variable. IMO this extra step was making things harder to follow.
…gument. Again, this is a step towards unification. This makes a clarity improvement possible - now we can remove the `asanBuild` default argument from cmake-configure-build.yml. IMO having defaults at different "levels" was very confusing.
Clarity improvement: Don't default it at the cmake-configure-build.yml level. Both native-build-test.yml and cross-build.yml now take it as a parameter, defaulting to empty, and pass it down. Finally, azure-pipelines.yml passes `cmakeAdditionalFlags: '-DTESTS_BUILD_ONLY=ON'` when performing cross builds. This is the final step towards unification.
…d-and-test.yml. Saying "and" in the name clarifies that we're doing two separate things.
This is the default on Windows. See: https://learn.microsoft.com/en-us/azure/devops/pipelines/tasks/reference/powershell-v2?view=azure-pipelines
Originally hardcoded to `ON`, now defaulted to `true`. (These are synonyms to CMake.)
These parameters (`llvmSHAVar` etc.) were storing the names of Azure Pipelines variables to create (`llvmSHA`), confusingly sharing the same names as PowerShell variables (`$llvmSHA` etc.). This layer of indirection was unnecessary. We can take the names `llvmSHAVar` etc. and directly use them as the names of Azure Pipelines variables.
…re Pipelines variables.
Alongside 'Code Format', we begin by fanning out an 'Early Build' for each architecture. This verifies that the STL builds, and also covers `/analyze` and the benchmarks (the latter is still skipped for plain ARM). It skips all testing, and saves more time by skipping the LLVM checkout. Then we fan in for the normal x64 build and test. This results in several improvements: * Any build breaks (whether ordinary, `/analyze`, or benchmark) are caught early and cheaply. Previously, we'd spend at least 8 shards, or the full 32 for a non-x64 break. * This reduces the length of the critical path. We previously paid two `/analyze` and benchmark builds on the critical path (x64, then non-x64). Now we perform one `/analyze` and benchmark build simultaneously with 'Code Format' which takes almost as long, so it's nearly free. * By removing work from the 32 full test shards (`/analyze` was expensive although the benchmarks are currently cheap), we're reducing the surface area for eviction, and reducing the amount of work that needs to be rerun after evictions. If 'Code Format' fails, this performs a bit more work than before, but less than 1 full test shard.
We're still compiling the tests, just not running them.
…oogle-benchmark.
Move one occurrence from build-and-test.yml into checkout-sources.yml, so it consistently appears before `checkout: self`. Change the other occurrence in format-validation.yml to also use a single-line `if exist` command.
These settings observably improve our checkout behavior.
And we don't need to use this when configuring the benchmarks.
…ules.yml. Now format-validation.yml can use checkout-self.yml, avoiding duplication.
…le.yml. This extracts 3 copy-pasted cmd scripts into 1 PowerShell script. We no longer need to use Azure Pipelines variables to communicate the SHA. Each remote is now named submodule-upstream for uniformity, as the name doesn't matter. I've performed a few additional simplifications that I believe are proper, but we'll need to watch out for problems when agents reuse repos: * After top-level self-checkout, each submodule directory should exist, so we shouldn't have to force-create it. * If the .git directory doesn't exist in the submodule, we shouldn't need to obliterate all other files there. We're going to perform a checkout and clean that should restore us to a known good state. * If the .git directory already exists, running `git init` again is harmless by design. * Instead of the "run `git remote get-url` and look for failure" technique, we can check the output of `git remote` to make `git remote add` idempotent.
See: https://learn.microsoft.com/en-us/powershell/module/az.compute/new-azvmconfig?view=azps-11.5.0#-maxprice Let's drop this since we won't need it if we ever go back to Spot VMs, and I'm worried it might conflict with Regular VMs.
/azp run STL-ASan-CI |
This comment was marked as resolved.
This comment was marked as resolved.
StephanTLavavej
changed the title
Azure Pipelines refactoring WIP
Toolset update: VS 2022 17.10 Preview 4, non-spot VMs, Azure Pipelines overhaul
Apr 17, 2024
CaseyCarter
approved these changes
Apr 18, 2024
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.
This will make it much less painful to work on the pipelines in the future. Thanks!
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Changelog
📖 Non-Changelog Summary
This is a response to the increasing eviction rate of spot VMs that we've recently been suffering. It changes our pool to regular (non-spot) VMs, which will result in far more reliable PR/CI builds. Regular VMs are ~2.5x more expensive for us than spot VMs, although this will be inherently mitigated by fewer reruns being necessary.
To further mitigate this cost increase, I've dramatically overhauled our Azure Pipelines machinery. Now, in parallel with the Code Format stage, we'll run 4 Early Build stages, verifying that the STL itself builds for each architecture. This also extracts building the STL with
/analyze
(which is significantly slower than an ordinary build), and building the benchmarks. If any of these builds fail, that indicates a severe problem that needs to be fixed before we run any of the tests. If the Code Format and Early Build stages pass, then we "fan in" and run the x64 build/tests, followed by "fan out" to non-x64 build/tests as usual. The full build/test stages (across 32 shards) are somewhat faster due to the/analyze
and benchmark builds being lifted out.The
/analyze
build imposes approximately 1 minute of additional cost, so overall we're saving ~32 minutes of compute across the full build/test stages. The Early Builds take about 3 minutes per architecture, so we're paying ~12 minutes there, so there should be a ~20 minute net savings for a fully successful run. In the event that an Early Build finds a problem, we save tons of compute (either 8 full shards for an x64 failure, or the whole 32 shards for a non-x64 failure). Note that failed Code Format validation now spends more compute, but I felt that this was an acceptable tradeoff. The overall critical path is mostly unchanged (we save ~2 minutes by avoiding two/analyze
costs, but the ~3 minutes for an Early Build takes longer than the ~1.5 minutes for Code Format).I had to massively overhaul the machinery to make this restructuring feasible. This overhaul became a top-level improvement by itself (at least for maintainers) - by extracting repetition and eliminating unnecessary logic, I saved ~40 net lines even while adding all of the Early Build logic. The result should be far more maintainable and extensible.
⚙️ Azure Pipelines Commits
testParallelism
; lit uses all CPUs by default.amd64
tox64
; VsDevCmd.bat handles them as synonyms.vsDevCmdArch
totargetArch
.vsDevCmdArch
intohostArch
andtargetArch
.hostArch: x64
to cross-build.yml.targetPlatform
; it was identical totargetArch
.hostArch
beforetargetArch
.displayName
is unnecessary.testSelection
toctestOptions
.ctestOptions
to a parameter with a default argument.'--tests-regex stlasan'
into a variable.asanBuild
as a parameter with a default argument.asanBuild
default argument from cmake-configure-build.yml. IMO having defaults at different "levels" was very confusing.cmakeAdditionalFlags
is defaulted.cmakeAdditionalFlags: '-DTESTS_BUILD_ONLY=ON'
when performing cross builds.hostArch
,targetArch
.buildBenchmarks
default fromtrue
tofalse
.PowerShell@2
task.pwsh: false
.task: CmdLine@2
to thescript:
shortcut.analyzeBuild
to controlSTL_USE_ANALYZE
.ON
, now defaulted totrue
. (These are synonyms to CMake.)analyzeBuild
default fromtrue
tofalse
.analyzeBuild
in asan-pipeline.yml.llvmSHAVar
etc.) were storing the names of Azure Pipelines variables to create (llvmSHA
), confusingly sharing the same names as PowerShell variables ($llvmSHA
etc.). This layer of indirection was unnecessary. We can take the namesllvmSHAVar
etc. and directly use them as the names of Azure Pipelines variables.git submodule status
into a loop.cd $(Build.SourcesDirectory)
as that's their defaultworkingDirectory
.doTesting
to control whether we checkout LLVM and build/run tests.skipTesting
below, but rewriting history was too much effort. Sorry for the extra complexity.)/analyze
and the benchmarks (the latter is still skipped for plain ARM). It skips all testing, and saves more time by skipping the LLVM checkout. Then we fan in for the normal x64 build and test. This results in several improvements:/analyze
, or benchmark) are caught early and cheaply. Previously, we'd spend at least 8 shards, or the full 32 for a non-x64 break./analyze
and benchmark builds on the critical path (x64, then non-x64). Now we perform one/analyze
and benchmark build simultaneously with 'Code Format' which takes almost as long, so it's nearly free./analyze
was expensive although the benchmarks are currently cheap), we're reducing the surface area for eviction, and reducing the amount of work that needs to be rerun after evictions.buildBenchmarks
control whether we checkout google-benchmark.git sparse-checkout
now defaults to--cone
.checkout: self
.if exist
command.fetchDepth: 1
andfetchTags: false
.cmakeAdditionalFlags
with specifictestsBuildOnly
.skipTesting
with a default offalse
.git init
again is harmless by design.git remote get-url
and look for failure" technique, we can check the output ofgit remote
to makegit remote add
idempotent.🧰 Toolset Update Commits
New-AzVMConfig
defaults to-MaxPrice -1
.