adding validation errors when the benchmarks are unsupported #2148

emanuel-v-r · 2022-10-13T20:22:31Z

Aims to fix #989, added a similar test to the one that is described in the issue.
After this PR, the summary will include validation errors and the output will look like this:

  Standard Output: 
// Benchmark BenchmarkAllCases.InvokeOnceVoid: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

// Benchmark BenchmarkAllCases.InvokeOnceTaskAsync: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

// Benchmark BenchmarkAllCases.InvokeOnceRefType: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

// Benchmark BenchmarkAllCases.InvokeOnceValueType: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

// Benchmark BenchmarkAllCases.InvokeOnceTaskOfTAsync: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

// Benchmark BenchmarkAllCases.InvokeOnceValueTaskOfT: Dry(Platform=X86, Toolchain=InProcessToolchain, InvocationCount=16, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=16, WarmupCount=1)
// cannot be run in-process. Validation errors:
//    * Job Dry, EnvironmentMode.Platform was run as X64 (X86 expected). Fix your test runner options.

No suitable benchmarks were found

Although I believe it fits the purpose of what is described in the original issue, the error is not that specific, if we want to have something more specific we will have to introduce a new method in the IToolChain interface as the existing one returns only a bool https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Toolchains/IToolchain.cs#L16

dnfadmin · 2022-10-13T20:22:45Z

All CLA requirements met.

YegorStepanov · 2022-10-14T09:32:22Z

Yep, we need a specific error message in summary.ValidationErrors.

The problem is that the benchmarks are filtered by Toolchain.IsSupported before the validation executes.

var supportedBenchmarks = GetSupportedBenchmarks(benchmarkRunInfos, compositeLogger, resolver); //filtering by Toolchain.IsSupported
if (!supportedBenchmarks.Any(benchmarks => benchmarks.BenchmarksCases.Any()))
    return new[] { Summary.NothingToRun(title, resultsFolderPath, logFilePath) };

var validationErrors = Validate(supportedBenchmarks, compositeLogger);
if (validationErrors.Any(validationError => validationError.IsCritical))
    return new[] { Summary.ValidationFailed(title, resultsFolderPath, logFilePath, validationErrors) };

if we want to have something more specific we will have to introduce a new method in the IToolChain interface as the existing one returns only a bool https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Toolchains/IToolchain.cs#L16

If we replace ILogger logger with List<ValidationError> validationErrors, we can do smth like this:

var supportedBenchmarks = GetSupportedBenchmarks(benchmarkRunInfos, resolver, out List<ValidationErrors> validationErrors);
validationErrors.AddRange(Validate(supportedBenchmarks, compositeLogger));

if (validationErrors.Any(validationError => validationError.IsCritical))
    return new[] { Summary.ValidationFailed(title, resultsFolderPath, logFilePath, validationErrors) };

if (!supportedBenchmarks.Any(benchmarks => benchmarks.BenchmarksCases.Any()))  // looks redundant, validators should check it
    return new[] { Summary.NothingToRun(title, resultsFolderPath, logFilePath) };

For the maintainers:
Validators are pretty bad at the moment, they need to be improved in some places. Someday I will write a "plan" issue. But the task is quite big.

emanuel-v-r · 2022-10-14T12:37:21Z

Yep, we need a specific error message in summary.ValidationErrors.

The problem is that the benchmarks are filtered by Toolchain.IsSupported before the validation executes.
var supportedBenchmarks = GetSupportedBenchmarks(benchmarkRunInfos, compositeLogger, resolver); //filtering by Toolchain.IsSupported
if (!supportedBenchmarks.Any(benchmarks => benchmarks.BenchmarksCases.Any()))
    return new[] { Summary.NothingToRun(title, resultsFolderPath, logFilePath) };

var validationErrors = Validate(supportedBenchmarks, compositeLogger);
if (validationErrors.Any(validationError => validationError.IsCritical))
    return new[] { Summary.ValidationFailed(title, resultsFolderPath, logFilePath, validationErrors) };
if we want to have something more specific we will have to introduce a new method in the IToolChain interface as the existing one returns only a bool https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Toolchains/IToolchain.cs#L16

If we replace ILogger logger with List<ValidationError> validationErrors, we can do smth like this:
var supportedBenchmarks = GetSupportedBenchmarks(benchmarkRunInfos, resolver, out List<ValidationErrors> validationErrors);
validationErrors.AddRange(Validate(supportedBenchmarks, compositeLogger));

if (validationErrors.Any(validationError => validationError.IsCritical))
    return new[] { Summary.ValidationFailed(title, resultsFolderPath, logFilePath, validationErrors) };

if (!supportedBenchmarks.Any(benchmarks => benchmarks.BenchmarksCases.Any()))  // looks redundant, validators should check it
    return new[] { Summary.NothingToRun(title, resultsFolderPath, logFilePath) };
For the maintainers: Validators are pretty bad at the moment, they need to be improved in some places. Someday I will write a "plan" issue. But the task is quite big.

Thanks for the feedback, changed accordingly here 524dbf9.
Notes:

I haven't replaced logger because it's being used inside the toolchain methods (ex: https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Toolchains/InProcess/InProcessValidator.cs#L107), seems that the lib needs some more generic approach for validations/errors, so that we don't need to span "writeline" all over the place
I have kept the original method "IsSupported" untouched, so that we don't break any contract

Please let me know if that works for you

adamsitnik

@emanuel-v-r big thanks for your contribution! PTAL at my comments

src/BenchmarkDotNet/Loggers/LoggerExtensions.cs

src/BenchmarkDotNet/Reports/Summary.cs

tests/BenchmarkDotNet.IntegrationTests/InProcessTest.cs

src/BenchmarkDotNet/Toolchains/IToolchain.cs

src/BenchmarkDotNet/Toolchains/CoreRun/CoreRunToolchain.cs

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

YegorStepanov · 2022-10-14T13:26:19Z

I guess this test won't contain validationError:

[Fact]
public void BenchmarkDifferentPlatformReturnsValidationError()
{
    var сonfig = new ManualConfig()
        .With(Job.Dry.With(InProcessToolchain.Instance).With(Platform.X86))
        .With(Job.Dry.With(InProcessToolchain.Instance).With(Platform.X64))
        .With(new OutputLogger(Output))
        .With(DefaultColumnProviders.Instance);

    var runInfo = BenchmarkConverter.TypeToBenchmarks(typeof(BenchmarkAllCases), сonfig);
    var summary = BenchmarkRunner.Run(runInfo);

    Assert.NotEmpty(summary.ValidationErrors);
}

Also, we need to add a specific reason to the validation errors.

To fix it we need change interface to it

public interface IToolchain                                                            
{                                                                                     
    [PublicAPI] string Name { get; }                                                   
    IGenerator Generator { get; }                                                      
    IBuilder Builder { get; }                                                          
    IExecutor Executor { get; }                                                        
    bool IsInProcess { get; }                                                          
                                                                                      
-   bool IsSupported(BenchmarkCase benchmarkCase, ILogger logger, IResolver resolver);
+   bool IsSupported(BenchmarkCase benchmarkCase, IResolver resolver, List<ValidationError> validationErrors);
}

and do it for each toolchain:

- logger.WriteLineError("The Roslyn toolchain is only supported on .NET Framework");
+ validationErrors.Add(new ValidationError(false, "The Roslyn toolchain is only supported on .NET Framework"));

But:

We need approval from the maintainers to do it.
Refactoring is desirable here, but it requires an understanding of the project's code
2.1) For example: change List<ValidationError> to the add-only collection ValidationErrors in the whole project.

emanuel-v-r · 2022-10-14T15:35:01Z

I guess this test won't contain validationError:

[Fact]
public void BenchmarkDifferentPlatformReturnsValidationError()
{
    var сonfig = new ManualConfig()
        .With(Job.Dry.With(InProcessToolchain.Instance).With(Platform.X86))
        .With(Job.Dry.With(InProcessToolchain.Instance).With(Platform.X64))
        .With(new OutputLogger(Output))
        .With(DefaultColumnProviders.Instance);

    var runInfo = BenchmarkConverter.TypeToBenchmarks(typeof(BenchmarkAllCases), сonfig);
    var summary = BenchmarkRunner.Run(runInfo);

    Assert.NotEmpty(summary.ValidationErrors);
}

Also, we need to add a specific reason to the validation errors.

To fix it we need change interface to it

public interface IToolchain                                                            
{                                                                                     
    [PublicAPI] string Name { get; }                                                   
    IGenerator Generator { get; }                                                      
    IBuilder Builder { get; }                                                          
    IExecutor Executor { get; }                                                        
    bool IsInProcess { get; }                                                          
                                                                                      
-   bool IsSupported(BenchmarkCase benchmarkCase, ILogger logger, IResolver resolver);
+   bool IsSupported(BenchmarkCase benchmarkCase, IResolver resolver, List<ValidationError> validationErrors);
}

and do it for each toolchain:

- logger.WriteLineError("The Roslyn toolchain is only supported on .NET Framework");
+ validationErrors.Add(new ValidationError(false, "The Roslyn toolchain is only supported on .NET Framework"));

But:

We need approval from the maintainers to do it.
Refactoring is desirable here, but it requires an understanding of the project's code
2.1) For example: change List<ValidationError> to the add-only collection ValidationErrors in the whole project.

I was not able to replicate the issue using your test but I do understand what the issue is, nice catch thank you!
This commit should do the trick c45077d.
Also @adamsitnik thanks for your feedback, I addressed most of your comments, but I kept open some of them that I still have some doubts.
More specifically this one #2148 (comment).
Seems that we all agree that this validate method should not log/print anything, and therefore the logger should be removed.
The problem is that inside the toolchain implementations, we do have some logging/printing, and there's no existing generic approach for such error handling.
My suggestion is that we create a separated issue for handling this, as it could take some effort.
Anyway the changes that I have done in this PR will not affect such behavior.
I can take it myself as the next issue, as I already have some context.

EDIT:
I ended up removing logger dependency from toolchain.
Please @YegorStepanov @adamsitnik take a look.

src/BenchmarkDotNet/Toolchains/InProcess.NoEmit/InProcessNoEmitToolchain.cs

src/BenchmarkDotNet/Loggers/LoggerExtensions.cs

YegorStepanov · 2022-10-14T16:10:02Z

src/BenchmarkDotNet/Toolchains/CsProj/CsProjClassicNetToolchain.cs

            }

            if (InvalidCliPath(customDotNetCliPath: null, benchmarkCase, logger))
-                return false;
+            {
+                var validationError = new ValidationError(true, $"InvalidCliPath, benchmark '{benchmarkCase.DisplayInfo}' will not be executed", benchmarkCase);


Do not use fast exit, collect all errors and return them.

It's better to display as much as possible errors.
If the user is not on windows AND the cli path is invalid, it displays only the first message.

I am not sure if we want to check all the errors, or just fail fast here, anyway I kept the existing behavior, not sure if we want to change that.

You can open the validators folder, all of them may display multiple errors.

You can open the validators folder, all of them may display multiple errors.

I mean toolchains errors, currently it only prints one error and exits as you can see in the previous code for "IsSupported" method
I am ok to change it as well, but I am trying to be objective here, and change only the necessary for what is described in the issue, and try not have side effects
Maybe @adamsitnik can provide his opinion here.

I like the idea of returning all errors instead the first one. 👍

Currently if there are two or more errors:

The users tries to run the benchmarks, gets a single error and fixes it.

The users tries to run the benchmarks, gets another error and fixes it.

The user actually runs the benchmarks.

If we provide all errors at once we can reduce the number of steps needed and hence improve the UX.

src/BenchmarkDotNet/Validators/ValidationError.cs

src/BenchmarkDotNet/Toolchains/CsProj/CsProjClassicNetToolchain.cs

adamsitnik

Overall, it looks very good, but it would be great if you could change the validator behavior to return all known errors when possible. I know that the original issue did not mention it, but since you are improving the error handling, why not make it even better when we can?

Thank you @emanuel-v-r !

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

src/BenchmarkDotNet/Toolchains/CoreRun/CoreRunToolchain.cs

src/BenchmarkDotNet/Toolchains/Mono/MonoAotToolchain.cs

src/BenchmarkDotNet/Toolchains/CoreRun/CoreRunToolchain.cs

adamsitnik · 2022-10-17T14:10:55Z

src/BenchmarkDotNet/Toolchains/CsProj/CsProjClassicNetToolchain.cs

            }

            if (InvalidCliPath(customDotNetCliPath: null, benchmarkCase, logger))
-                return false;
+            {
+                var validationError = new ValidationError(true, $"InvalidCliPath, benchmark '{benchmarkCase.DisplayInfo}' will not be executed", benchmarkCase);


I like the idea of returning all errors instead the first one. 👍

Currently if there are two or more errors:

The users tries to run the benchmarks, gets a single error and fixes it.

The users tries to run the benchmarks, gets another error and fixes it.

The user actually runs the benchmarks.

If we provide all errors at once we can reduce the number of steps needed and hence improve the UX.

src/BenchmarkDotNet/Toolchains/Toolchain.cs

* Replace IsSupported method by Validate which returns the errors instead of only a bool * Extract printing logic from tool chains * Remove logger dependency from IToolChain Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

emanuel-v-r · 2022-10-17T17:39:55Z

Overall, it looks very good, but it would be great if you could change the validator behavior to return all known errors when possible. I know that the original issue did not mention it, but since you are improving the error handling, why not make it even better when we can?

Thank you @emanuel-v-r !

Thank you @adamsitnik , applied your suggestions, and also changed the validate method in the other toolchains so that it returns multiple erros.

YegorStepanov · 2022-10-17T18:03:56Z

src/BenchmarkDotNet/Running/BenchmarkRunnerDirty.cs

@@ -136,7 +136,7 @@ private static Summary RunWithExceptionHandling(Func<Summary> run)
            catch (InvalidBenchmarkDeclarationException e)
            {
                ConsoleLogger.Default.WriteLineError(e.Message);
-                return Summary.NothingToRun(e.Message, string.Empty, string.Empty);
+                return Summary.ValidationFailed(e.Message, string.Empty, string.Empty);


Looks a little weird here. Maybe find a better name?

Any suggestion? Maybe just Summary.Failed?

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

Co-authored-by: Yegor Stepanov <yegor.stepanov@outlook.com>

src/BenchmarkDotNet/Toolchains/Mono/MonoAotToolchain.cs

adamsitnik

LGTM, thank you very much for your contribution @emanuel-v-r !

adamsitnik · 2022-10-17T19:48:03Z

src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs


 namespace BenchmarkDotNet.Toolchains.MonoWasm
 {
    [PublicAPI]
-    public class WasmToolChain : Toolchain
+    public class WasmToolchain : Toolchain


Renaming public types is considered to be a breaking change and we avoid doing that if we can. However, this particular type is most likely not used directly by anyone, so it's OK-ish ;)

adamsitnik · 2022-10-17T19:49:45Z

src/BenchmarkDotNet/Validators/ValidationError.cs

@@ -50,6 +51,7 @@ public override int GetHashCode()
            }
        }

+


nit: redundant empty lone

emanuel-v-r force-pushed the validation-errors-for-unsupported-benchmarks branch 3 times, most recently from b08a45b to 3863545 Compare October 14, 2022 13:06

adamsitnik reviewed Oct 14, 2022

View reviewed changes

emanuel-v-r force-pushed the validation-errors-for-unsupported-benchmarks branch from 1e54eb9 to bbc9544 Compare October 14, 2022 14:03

YegorStepanov reviewed Oct 14, 2022

View reviewed changes

emanuel-v-r force-pushed the validation-errors-for-unsupported-benchmarks branch from 4d594fc to 86e07f5 Compare October 15, 2022 12:53

adamsitnik reviewed Oct 17, 2022

View reviewed changes

YegorStepanov reviewed Oct 17, 2022

View reviewed changes

src/BenchmarkDotNet/Toolchains/Toolchain.cs Show resolved Hide resolved

emanuel-v-r force-pushed the validation-errors-for-unsupported-benchmarks branch 2 times, most recently from ffa1224 to 5953396 Compare October 17, 2022 16:47

Refactoring tool chains in order to return validation errors

e3ed1e1

* Replace IsSupported method by Validate which returns the errors instead of only a bool * Extract printing logic from tool chains * Remove logger dependency from IToolChain Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

emanuel-v-r force-pushed the validation-errors-for-unsupported-benchmarks branch from 5953396 to e3ed1e1 Compare October 17, 2022 17:31

YegorStepanov reviewed Oct 17, 2022

View reviewed changes

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs Outdated Show resolved Hide resolved

Update src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

aff0828

Co-authored-by: Yegor Stepanov <yegor.stepanov@outlook.com>

YegorStepanov reviewed Oct 17, 2022

View reviewed changes

src/BenchmarkDotNet/Toolchains/Mono/MonoAotToolchain.cs Outdated Show resolved Hide resolved

styling validation errors

17857b2

adamsitnik approved these changes Oct 17, 2022

View reviewed changes

adamsitnik added this to the v0.13.3 milestone Oct 17, 2022

adamsitnik added the Area:Validators label Oct 17, 2022

adamsitnik merged commit 28bf214 into dotnet:master Oct 17, 2022

YegorStepanov mentioned this pull request Oct 29, 2022

Improve appearance of validation output #2179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding validation errors when the benchmarks are unsupported #2148

adding validation errors when the benchmarks are unsupported #2148

emanuel-v-r commented Oct 13, 2022 •

edited

dnfadmin commented Oct 13, 2022 •

edited

YegorStepanov commented Oct 14, 2022

emanuel-v-r commented Oct 14, 2022 •

edited

adamsitnik left a comment

YegorStepanov commented Oct 14, 2022

emanuel-v-r commented Oct 14, 2022 •

edited

YegorStepanov Oct 14, 2022

emanuel-v-r Oct 14, 2022 •

edited

YegorStepanov Oct 14, 2022

emanuel-v-r Oct 14, 2022

adamsitnik Oct 17, 2022

adamsitnik left a comment

adamsitnik Oct 17, 2022

emanuel-v-r commented Oct 17, 2022 •

edited

YegorStepanov Oct 17, 2022

emanuel-v-r Oct 17, 2022 •

edited

adamsitnik left a comment

adamsitnik Oct 17, 2022

adamsitnik Oct 17, 2022

adding validation errors when the benchmarks are unsupported #2148

adding validation errors when the benchmarks are unsupported #2148

Conversation

emanuel-v-r commented Oct 13, 2022 • edited

dnfadmin commented Oct 13, 2022 • edited

YegorStepanov commented Oct 14, 2022

emanuel-v-r commented Oct 14, 2022 • edited

adamsitnik left a comment

Choose a reason for hiding this comment

YegorStepanov commented Oct 14, 2022

emanuel-v-r commented Oct 14, 2022 • edited

Choose a reason for hiding this comment

emanuel-v-r Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuel-v-r commented Oct 17, 2022 • edited

Choose a reason for hiding this comment

emanuel-v-r Oct 17, 2022 • edited

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuel-v-r commented Oct 13, 2022 •

edited

dnfadmin commented Oct 13, 2022 •

edited

emanuel-v-r commented Oct 14, 2022 •

edited

emanuel-v-r commented Oct 14, 2022 •

edited

emanuel-v-r Oct 14, 2022 •

edited

emanuel-v-r commented Oct 17, 2022 •

edited

emanuel-v-r Oct 17, 2022 •

edited