Skip to content

Conversation

@ryanlai2
Copy link
Contributor

@ryanlai2 ryanlai2 commented Mar 6, 2019

This PR pulls out the code in EvaluateModels that loops through all inputBindingTypes inputDataTypes and deviceCreationLocations into a separate function.

EvaluateModels loops through all deviceTypes and calls EvaluateModelGivenDeviceType

@ryanlai2 ryanlai2 requested a review from a team as a code owner March 6, 2019 00:15
@ryanlai2 ryanlai2 requested a review from yskim1501 March 6, 2019 00:15
@ryanlai2 ryanlai2 changed the title User/rylai/run on all devices Run on all devices even if other device types fail Mar 6, 2019
Copy link
Contributor

@yskim1501 yskim1501 left a comment

Choose a reason for hiding this comment

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

LGTM

{
for (const auto& inputBindingType : inputBindingTypes)
HRESULT evaluateModelGivenDeviceTypeResult =
EvaluateModelGivenDeviceType(model, deviceType, inputBindingTypes, inputDataTypes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to EvaluateModelWithDeviceType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure sure! I can rename this.

}
lastEvaluateModelResult = evaluateModelGivenDeviceTypeResult;
std::cout << "Run failed for DeviceType: " << TypeHelper::Stringify(deviceType) << std::endl;
;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra ; :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@yskim1501
Copy link
Contributor

It would be great if we can add test cases for this, but I understand that this might be hard to repro (since you should purposely fail in one of the devices, which is not what where should be at all)

@ryanlai2
Copy link
Contributor Author

ryanlai2 commented Mar 6, 2019

It would be great if we can add test cases for this, but I understand that this might be hard to repro (since you should purposely fail in one of the devices, which is not what where should be at all)

Right, it would be great to add test cases for this. However, it would be hard to find a model that purposely fails on one of the devices but passes on others.

In the future, I believe using mocking framework would be best to test this.

@ryanlai2 ryanlai2 merged commit 42ee5b4 into master Mar 6, 2019
@ryanlai2 ryanlai2 deleted the user/rylai/run_on_all_devices branch March 6, 2019 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants