-
Notifications
You must be signed in to change notification settings - Fork 789
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
Turn a bunch of options by default #17442
Conversation
✅ No release notes required |
Thanks @vzarytovskii for looking into this. I failed in my attempt in #17400 |
A bunch of tests currently failing:
I will turn parallel resolution off and see what still fails. |
Majority of issues were due to parallel resolution it seems. Still doesn't explain why do I have weird issues in tests locally, but not in CI. It's good. I will leave parallel ilxgen and resolution off, and graph type checking and optimization on, and leave it up to @KevinRansom and @T-Gro to decide whether we want them on by default. The latter should be fine, the former - we didn't test on many solutions (maybe 3-5 in the wild), so there's always a risk of things going wrong. There's now an opt-out mechanism though. |
This exists already: i.e. deterministic builds should not be affected. |
@T-Gro @KevinRansom @abonie @psfinaki I need a sanity check on it. Also, a sign-off. |
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.
Looks good, thanks for the follow-up.
I think it makes sense to include this for previews and release candidates.
This gives us enough time to reconsider the defaults before November if needed.
So, two things broke when turning this on by default
Line 1488 in c9bed79
fsharp/tests/fsharp/core/topinit/app69514.fs Lines 1 to 5 in c9bed79
fsharp/tests/fsharp/core/topinit/lib69514.fs Lines 1 to 13 in c9bed79
Fails with:
fsharp/tests/fsharp/TypeProviderTests.fs Lines 39 to 41 in c9bed79
@nojaf any pointers what to look at in this case? |
@vzarytovskii , I'm gonna teach you a little trick, add the scenario to
and then run fsharp/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs Lines 49 to 55 in c9bed79
if sequential passes and graph doesn't then it is indeed a graph problem. I need to go now, but I can look into this on Monday. SynLongIdent.SynLongIdent(
id = [
Ident("`global`", R("(2,9--2,15)"))
Ident("Lib", R("(2,16--2,19)"))
Ident("File1", R("(2,20--2,25)"))
Ident("discState", R("(2,26--2,35)"))
Ident("Second", R("(2,36--2,42)"))
],
dotRanges = [ R("(2,15--2,16)"); R("(2,19--2,20)"); R("(2,25--2,26)"); R("(2,35--2,36)") ],
trivia = [ Some(IdentTrivia.OriginalNotation("global")); None; None; None; None ]
), |
This what I initially noticed in the test. Let me try and tinker with it. |
Added test here: #17553, will look at fixing it (need to learn a bit more about how graph checking works). |
Hi @vzarytovskii is the plan still to have this for November? |
I will try to wrap it, but it might have to do in the following release. Too many issues with this release and didn't have time to finish this one :( |
Would that be minor or major? |
Minor, as soon as sdk freeze is lifted |
Ok, probably not going to make it till EOD, there are some fixes need to be made to the graph checking, will look at those:
|
What tests are you running there? Is that only using graph-based checking or all settings? |
Those are fsharpqa tests, Seems like many of them are related to relative # resolution ("#I/#r" I presume?). |
Could you try and just run it with |
Will try it, but it's unlikely an optimizer, since the issues are during checking and resolution. Optimizer ones would've popped up at a later phase. Also i think that by default fsharpqa is compiling debug/without optimizations. I will double check it though. |
Superseded by #17948 |
Maybe it's nothing but it caught my eye, so I'll put it here before I forget: fsharp/src/Compiler/Driver/GraphChecking/GraphProcessing.fs Lines 113 to 115 in 658b245
to signal things are done IIUC. But it is called here: fsharp/src/Compiler/Driver/GraphChecking/GraphProcessing.fs Lines 146 to 147 in 658b245
after queueNode started some work with Async.Start :fsharp/src/Compiler/Driver/GraphChecking/GraphProcessing.fs Lines 117 to 118 in 658b245
I wonder if maybe some things are not really done when the cancellation happens? |
Attempt to enable some parallel options (graph checking, ilxgen, resolution) by default.
Firstly, based on local tests, parallel ilxgen causes a bunch of baselines to change. This either caused by sorting before emitting/generating, which is fine. Or it's caused by trully parallel generation, which will break determinism.
Secondly, graph checking causes some things in tests to be not found during test (i'd wait for CI, to see exact issue).
I am very hesitant to enable graph checking by default, since it might cause typechecker just to fail in places it wasn't before, there might be (and probably will be) a bunch of cases where it will surface. What we can do is to have an msbuild property which will allow people to test it on more projects (or ask them to use
--test:
flag).Regarding the ilxgen - we need a proof that codegen order is expected. If it's not, it has to be fixed, or always been disabled for the determinism.
I didn't find any anomalies in both
ParallelReferenceResolution
andOptimizationProcessingMode.Parallel
yet.