Don't include the SDK in our helix payload #6918
Merged
Conversation
This file contains hidden or 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
I noticed that the tests included the latest SDK - including the host - in our helix payloads. This is a large amount of unnecessary downloads and it also makes it so we use the latest host on the older frameworks which can fail when the latest host drops support for distros. Since our tests shouldn't need the full CLI, remove this from our helix payloads. We'll instead get just the runtime we need through `AdditionalDotNetPackage`
ghost
assigned 
|
I noticed that we might not get So we might need to tweak our command line for executing the tests: machinelearning/eng/helix.proj Line 151 in 5483ba9 |
|
Yep, right on time as I discovered this the tests hit it. I do see this is having the desired effect (runtime added, but no SDK): https://helixde107v0xdeko0k025g8.blob.core.windows.net/helix-job-f33c5c7d-ade1-462f-bbf7-9c7eceea97510e66aa157474e3aad/job-list-92e85b36-8b0e-416c-b245-155dc7a13ad0.json?helixlogtype=result Now to get |
Helix only sets the path when the CLI is included, however we don't need the CLI.
michaelgsharp
approved these changes
Dec 20, 2023
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6918 +/- ##
=======================================
Coverage 68.79% 68.79%
=======================================
Files 1249 1249
Lines 249431 249431
Branches 25510 25510
=======================================
+ Hits 171604 171606 +2
Misses 71214 71214
+ Partials 6613 6611 -2
Flags with carried forward coverage won't be shown. Click here to find out more. |
ericstj
added a commit
that referenced
this pull request
Jan 5, 2024
* Don't include the SDK in our helix payload I noticed that the tests included the latest SDK - including the host - in our helix payloads. This is a large amount of unnecessary downloads and it also makes it so we use the latest host on the older frameworks which can fail when the latest host drops support for distros. Since our tests shouldn't need the full CLI, remove this from our helix payloads. We'll instead get just the runtime we need through `AdditionalDotNetPackage` * Place Helix downloaded runtime on the PATH Helix only sets the path when the CLI is included, however we don't need the CLI.
michaelgsharp
added a commit
that referenced
this pull request
Jan 9, 2024
* Update dependencies from https://github.com/dotnet/arcade build 20231220.2 Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild , Microsoft.DotNet.XUnitExtensions From Version 8.0.0-beta.23265.1 -> To Version 8.0.0-beta.23620.2 * Fixed version update breaks. * Update XUnitVersion * Update MicrosoftMLOnnxRuntimeVersion to 1.16.3 * Rollback OnnxRuntime and suppress warning * Update to Xunit with fix for xunit/xunit#2821 * Update Centos docker containers * Fix packaging step * Try including stdint.h to fix missing uint8_t on centos * Update Centos test queue * Attempt to use runtime centos-stream8-helix container for tests * Use centos-stream8-mlnet-helix container for testing * Undo changes to test data * Make NETFRAMEWORK ifdef versionless * Only use semi-colons for NoWarn * Fix assert by only accessing idx (#6924) Asserting on `_rowCount < Utils.Size(_valueBoundaries)` was catching a case where `_rowCount`'s update was reordered before `_valueBoundaries` This was unnecessary, since this method doesn't need to use `_rowCount`. Instead, make the asserts use only `idx` which will be maintained consistent with the waiter logic in this cache. �Ensure we only ever use `_rowCount` from the caching thread, so write reordering won't matter. * Don't include the SDK in our helix payload (#6918) * Don't include the SDK in our helix payload I noticed that the tests included the latest SDK - including the host - in our helix payloads. This is a large amount of unnecessary downloads and it also makes it so we use the latest host on the older frameworks which can fail when the latest host drops support for distros. Since our tests shouldn't need the full CLI, remove this from our helix payloads. We'll instead get just the runtime we need through `AdditionalDotNetPackage` * Place Helix downloaded runtime on the PATH Helix only sets the path when the CLI is included, however we don't need the CLI. * Make double assertions compare with tolerance instead of precision (#6923) Precision might cause small differences to round to a different number. Instead compare with a tolerance which is not sensitive to rounding. --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Michael Sharp <[email protected]> Co-authored-by: Eric StJohn <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
I noticed that the tests included the latest SDK - including the host - in our helix payloads.
This is a large amount of unnecessary downloads and it also makes it so we use the latest host on the older frameworks which can fail when the latest host drops support for distros.
Since our tests shouldn't need the full CLI, remove this from our helix payloads.
We'll instead get just the runtime we need through
AdditionalDotNetPackage