Skip to content
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

Target .NET 10 / net10.0 #106599

Draft
wants to merge 67 commits into
base: main
Choose a base branch
from
Draft

Conversation

ViktorHofer
Copy link
Member

Contributes to #105343

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Aug 18, 2024
@dotnet dotnet deleted a comment from dotnet-issue-labeler bot Aug 18, 2024
@dotnet dotnet deleted a comment from dotnet-issue-labeler bot Aug 18, 2024
@lewing lewing added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 19, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@carlossanlop
Copy link
Member

@ViktorHofer I already had this PR open for this purpose: #106421

@am11
Copy link
Member

am11 commented Sep 20, 2024

Still using dotnet/performance and main branch

image

maybe it is using these pipelines from runtime's main branch instead of PR's?

@ilonatommy
Copy link
Member

Shouldn't we change this line?

git clone https://github.com/dotnet/performance.git --depth 1 -b ${{ parameters.perfBranch }} --single-branch

@am11
Copy link
Member

am11 commented Sep 20, 2024

Ah, it was added four days ago b14e2f5, and the branch I was grepping in was a week old. 🤞

@ilonatommy
Copy link
Member

RuntimeError: Unable to determine the .NET SDK used for net9.0. SDKs found in /mnt/vss/_work/1/performance/CorrelationStaging/payload/dotnet/sdk: ['10.0.100-alpha.1.24468.12']. Major version: 9 build

@am11
Copy link
Member

am11 commented Sep 20, 2024

Yup, it's picked up. I have reapplied the workaround (which I removed to see if it's needed after the clone is fixed). Please retry.

@ilonatommy
Copy link
Member

ilonatommy commented Sep 20, 2024

Worked, performance is green. We should apply it to perf repo and then revert the changes to cloned url.

Edit: my bad, not fully green:
argparse.ArgumentTypeError: Version "10.0.100-alpha.1.24468.12" is in the wrong format
https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-106599-merge-4d8f794af50e426bb1/wasm.x64.micro.net10.0.Partition0/1/console.8002e11c.log?helixlogtype=result

Edit2: please, change
if not search(r'^\d\.\d+\.\d+', version):
to if not search(r'^\d+\.\d+\.\d+', version):
in line 124.

@ilonatommy
Copy link
Member

We're stuck on

dotnet build /datadisks/disk1/work/A6660968/w/BEFD0A5E/e/performance/src/benchmarks/micro/MicroBenchmarks.csproj --configuration Release --framework net9.0 --no-restore /p:NuGetPackageRoot=/datadisks/disk1/work/A6660968/w/BEFD0A5E/e/performance/artifacts/packages /p:RestorePackagesPath=/datadisks/disk1/work/A6660968/w/BEFD0A5E/e/performance/artifacts/packages /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 --output /datadisks/disk1/work/A6660968/w/BEFD0A5E/e/performance/artifacts/bin/for-running/MicroBenchmarks /p:BuildingForWasm=true
....
[2024/09/20 12:00:48][WARNING] Benchmark run for framework 'net9.0' contains errors

shouldn't we change the arg to be -f net10.0? I'm not sure where we're passing it from

@am11
Copy link
Member

am11 commented Sep 20, 2024

Is it so that before #101143, major version change wasn't getting blocked on perf leg? It could be we might have missed considering this detail during that refactoring work. cc @DrewScoggins, @caaavik-msft

One way to move forward could be to disable perf legs and enable it once the major version transition has been completed.

@lewing
Copy link
Member

lewing commented Sep 20, 2024

Is it so that before #101143, major version change wasn't getting blocked on perf leg? It could be we might have missed considering this detail during that refactoring work. cc @DrewScoggins, @caaavik-msft

One way to move forward could be to disable perf legs and enable it once the major version transition has been completed.

Typically, we continue to target 9 but install the net10 sdk during transition periods like this. I'd rather not disable the perf tests, it has lead to large gaps in testing in the past that included regressions that were extremely difficult to isolate.

@am11
Copy link
Member

am11 commented Sep 20, 2024

@lewing yup, what I was driving at is, I think before #101143,, we were able to do "use net(N) on net(N+1) in perf leg", now it's not possible, or maybe it requires a bit different approach? I haven't looked through last year PRs too closely yet.

@caaavik-msft
Copy link
Contributor

@lewing yup, what I was driving at is, I think before #101143,, we were able to do "use net(N) on net(N+1) in perf leg", now it's not possible, or maybe it requires a bit different approach? I haven't looked through last year PRs too closely yet.

This should still be possible. Looking at the wasm-runtime-perf build logs the only thing that was stopping this from working was the version parsing issue. I think if we just revert the changes to channel_map.py from dotnet/performance#4470 then it should work. If it doesn't, I'd be curious to know what the error is.

@am11
Copy link
Member

am11 commented Sep 20, 2024

If it doesn't, I'd be curious to know what the error is.

dotnet/performance#4470 (comment) <- this was the error before making changes in channel_map. Feel free to push to that PR and just rerun this leg https://dev.azure.com/dnceng-public/public/_build/results?buildId=813887&view=logs&j=59ba1e23-1c3f-5e83-bfa2-2b4c458b95b8&t=606691c3-9bf3-5776-e07f-204fdb23192e (it's already on attempt 7).

@caaavik-msft
Copy link
Contributor

After looking into it some more, I think the reason it isn't working is because of the new SDK validation feature (dotnet/BenchmarkDotNet#2523, dotnet/BenchmarkDotNet#2538) that is part of BenchmarkDotNet which will validate that the SDK is installed. The SDK validation feature was introduced 6 months ago and so was not an issue when we went from .NET 8 to 9.

@lewing
Copy link
Member

lewing commented Sep 20, 2024

After looking into it some more, I think the reason it isn't working is because of the new SDK validation feature (dotnet/BenchmarkDotNet#2523, dotnet/BenchmarkDotNet#2538) that is part of BenchmarkDotNet which will validate that the SDK is installed. The SDK validation feature was introduced 6 months ago and so was not an issue when we went from .NET 8 to 9.

Does this mean the perf runs will fail for all the runtimes but we're only seeing wasm fail because of our validation run?

@lewing
Copy link
Member

lewing commented Sep 20, 2024

Just a reminder that runtime flow into the sdk is stuck at dotnet/sdk#43061 (or dotnet/sdk#43070 ) and this is the next step in unblocking that.

@caaavik-msft
Copy link
Contributor

After looking into it some more, I think the reason it isn't working is because of the new SDK validation feature (dotnet/BenchmarkDotNet#2523, dotnet/BenchmarkDotNet#2538) that is part of BenchmarkDotNet which will validate that the SDK is installed. The SDK validation feature was introduced 6 months ago and so was not an issue when we went from .NET 8 to 9.

Does this mean the perf runs will fail for all the runtimes but we're only seeing wasm fail because of our validation run?

I ran our full performance test suite, and although it is still running I think it only affects wasm and nativeaot because for them we specify a custom runtime argument (link to source) which specifies which SDK to run against whereas for other runs like coreclr there is no SDK passed in, it just uses the corerun path.

We have a few options here:

  1. Make a PR to BDN to add wasmnet100 as a possible runtime. This would need to be synchronised with this PR to change the TFM to net10.0.
  2. Make a PR to BDN that allows for disabling the SDK validator
  3. It is possible to instead of passing in --runtimes wasmnet70 to pass in --runtimes wasm and it will use the same SDK that is used to run the benchmark project which might work. Unfortunately the BDN codebase has some code that uses this value to determine whether or not to load test-main.js or main.js as that changed in .NET 7. I was thinking a bit of a hacky workaround that might work is to make it so that we rename test-main.js to main.js just before calling BDN and so we wouldn't need to make any BDN changes to get it all working.

I'm going to see if option 3 works first, but if it doesn't then I'll probably proceed with option 2 which might take until Monday to get merged into BDN so we can start using it. One other thing is that it has been quite a while since we last updated our BDN version, I am concerned that if we pull the latest version of BDN that it might cause other things to break and it might take longer than we expected if other bug fixes need to be made. If that happens we might just make a custom build of BDN based off the current version we are using but only with the fix for the SDK validator.

@am11
Copy link
Member

am11 commented Sep 21, 2024

Make a PR to BDN that allows for disabling the SDK validator

This will scale better. Let SDK handle the validations and delete those dozen of back and forth mappings. If BDN’s excuse is that public moniker enum used in the attribute, it’s probably worth taking a breaking change; delete the enum and replace with parameterized one [SupportedMoniker(“net9.0”). Hardcoded lists of platform/framework names and versions is not a good design, and we seem to have just too many of them.

@lewing
Copy link
Member

lewing commented Sep 21, 2024

Agreed, whatever the solution let's assume the same thing will happen again next September and plan for that this time.

@caaavik-msft
Copy link
Contributor

Made the BDN PR here dotnet/BenchmarkDotNet#2641

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.