Skip to content

Conversation

@Anipik
Copy link
Contributor

@Anipik Anipik commented Dec 20, 2018

The benchmarks involving rsps don't fail even when the underlying rsp has thrown an exception

@Anipik Anipik requested review from TomFinley and sfilipi December 20, 2018 23:16
@Ivanidzo4ka
Copy link
Contributor

Out of curiosity, why we even use rsp?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 20, 2018

Out of curiosity, why we even use rsp?

Because when we introduced benchmarking, we wanted to have them asap. And converting them to api required someone on the ml team and it was not possible at that time

@Anipik
Copy link
Contributor Author

Anipik commented Dec 21, 2018

cc @danmosemsft @justinormont @adamsitnik

@TomFinley
Copy link
Contributor

Hi @Anipik thanks for writing this.

Minor point... so, I see this appears to be part of something called a Dispose method. Could we name it something else? I think naming it this when it's not actually a dispose method violates some sort of .NET design guideline (despite the fact that this does not implement IDisposable). It's at least needlessly confusing.

While we're at it, is there a reason to not make this base class abstract?

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Anipik !

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2018

@justinormont @Ivanidzo4ka can you review this one ?

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose a different approach: instead of introducing a base class type with [IterationCleanup] method that checks the exit code we could add a new helper method, which would call Maml.MainCore, check the exit code and throw in the benchmark when the exception happens.

sth like:

void ExecuteMamlCommand(this string command, IHostEnvironment environment)
{
    if (Maml.MainCore(environment, command, alwaysPrintStacktrace: false) < 0)
        throw new Exception($"Command {command} returned error code"); // + some details if possible
}

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@Anipik Anipik merged commit 7e21afa into dotnet:master Dec 27, 2018
@Anipik Anipik deleted the exception branch December 27, 2018 17:56
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants