Skip to content

[PP GAPI] Addded tests to cover exisiting precision conversions done by some plugins#1976

Merged
azhogov merged 1 commit intoopenvinotoolkit:masterfrom
anton-potapov:preproc_precision_conversion_tests
Sep 16, 2020
Merged

[PP GAPI] Addded tests to cover exisiting precision conversions done by some plugins#1976
azhogov merged 1 commit intoopenvinotoolkit:masterfrom
anton-potapov:preproc_precision_conversion_tests

Conversation

@anton-potapov
Copy link
Copy Markdown
Contributor

@anton-potapov anton-potapov commented Aug 28, 2020

To cover existing code of precision conversion in the plugins (i.e. TEMPLATE) with the test following was done

  • added shared parameterized tests
  • instantiated for template plugin
  • instantiated for CPU plugin
  • fixed CPU plugin to properly handle U16 input.
    • fixed U16 handling inconsistency between :
      • MKLDNNPlugin::Engine::LoadExeNetworkImpl , which convert all U16 nodes to I32
      • and MKLDNNPlugin::MKLDNNInferRequest::InferImpl , which were converting U16 to FP32 (changed to I32)
    • fixed reverse sequence primitive config to disallow non FP32 input/output (assumed that plugin will inserted needed conversions to the graph)

@anton-potapov anton-potapov added category: IE Tests OpenVINO Test: plugins and common category: preprocessing (deprecated) Inference Engine Preprocessing library labels Aug 28, 2020
@anton-potapov anton-potapov requested a review from a team as a code owner August 28, 2020 08:23
@anton-potapov anton-potapov requested review from a team, ilya-lavrenov and ilyachur August 28, 2020 08:23
@ilya-lavrenov
Copy link
Copy Markdown
Contributor

@anton-potapov can we extend for CPU plugin as well? Because template plugin tests are not running by CI at the moment..

@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch from 77851ff to f54baf4 Compare August 31, 2020 08:19
@anton-potapov
Copy link
Copy Markdown
Contributor Author

anton-potapov commented Sep 1, 2020

@anton-potapov can we extend for CPU plugin as well? Because template plugin tests are not running by CI at the moment..

@ilya-lavrenov sure we can. But the problem is that they are failing, not sure why :)

@ilya-lavrenov
Copy link
Copy Markdown
Contributor

@anton-potapov suggest to debug :) maybe the problems in tests + template plugin? I wonder if CPU plugin contains such bugs

@anton-potapov
Copy link
Copy Markdown
Contributor Author

@anton-potapov suggest to debug :) maybe the problems in tests + template plugin? I wonder if CPU plugin contains such bugs

@ilya-lavrenov IMHO it is the cpu plugin fault. So lets move with this one to enable #857 with examples

@ilya-lavrenov
Copy link
Copy Markdown
Contributor

@anton-potapov ok, can we add such tests for any other plugin? I just want to make sure that tests itself as correct.
E.g. maybe you have GPU or VPU stick in order to run your tests?

@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch from f54baf4 to e5046d1 Compare September 4, 2020 11:08
@anton-potapov anton-potapov requested review from a team and dmitry-gorokhov September 4, 2020 11:08
@anton-potapov
Copy link
Copy Markdown
Contributor Author

@anton-potapov ok, can we add such tests for any other plugin? I just want to make sure that tests itself as correct.
E.g. maybe you have GPU or VPU stick in order to run your tests?

Added tests for CPU plugin and fix for it. @dmitry-gorokhov could you please take a look ?

@ilya-lavrenov
Copy link
Copy Markdown
Contributor

@anton-potapov please, make sure tests and CI are green.

@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch 2 times, most recently from 47761c0 to adb48f0 Compare September 7, 2020 17:07

std::vector<size_t> inputShape {4, 4};

//Idea of the tests is
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor corrections (please check the 2nd bullet! It wasn't readable, but my fixes might've ruined the meaning):

// This test:
// - Uses a no-op graph to test the plugin internal precision preprocessing only.
// - Mimics the reference code preprocessing via an extra nGraph Convert operation.
// Two graphs are created: a graph for the plugin and a graph for the nGraph reference code. 

Also in the last one - who exactly creates the graphs? Some particular function? If so, let's state it

Copy link
Copy Markdown
Contributor Author

@anton-potapov anton-potapov Sep 11, 2020

Choose a reason for hiding this comment

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

Minor corrections (please check the 2nd bullet! It wasn't readable, but my fixes might've ruined the meaning):

// This test:
// - Uses a no-op graph to test the plugin internal precision preprocessing only.
// - Mimics the reference code preprocessing via an extra nGraph Convert operation.
// Two graphs are created: a graph for the plugin and a graph for the nGraph reference code. 

changed to :

    // This test:
    // - Strive to test the plugin internal preprocessing (precision conversion) only.
    //   Thus (logically) no-op graph is used.
    // - Reference code mimic the preprocessing via extra ngraph Convert operation.
    // - Create/uses two (different) graphs here : one to feed the the plugin and one calculate the reference result.

and moved the comment to the beginning of the function.

Also in the last one - who exactly creates the graphs? Some particular function? If so, let's state it

the graphs are created via helper lambda :

        function            = make_ngraph(false); 
        reference_function  = make_ngraph(true);  //use extra ops to mimic the preprocessing

Does it make more sense now ?

@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch from adb48f0 to 573074c Compare September 11, 2020 12:29
@anton-potapov
Copy link
Copy Markdown
Contributor Author

@dmitry-gorokhov , @aalborov ping

@ilya-lavrenov ilya-lavrenov modified the milestones: 2021.1, 2021.2 Sep 14, 2020
@ilya-lavrenov
Copy link
Copy Markdown
Contributor

@anton-potapov please, make CI green

@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch 2 times, most recently from f98463c to 7a3ece4 Compare September 15, 2020 14:32
@anton-potapov anton-potapov requested a review from a team September 15, 2020 14:32
some plugins

- added shared parameterized tests
- instantiated for template plugin
- instantiated for cpu plugin
- fixed CPU plugin to properly handle U16 input
- fixed CPU reverse_sequence primitive to alolw input/oputput tensors to
be in FP32 only
- updated ngraph test_simple_computation_on_ndarrays to not expect
failure on U16 input
@anton-potapov anton-potapov force-pushed the preproc_precision_conversion_tests branch from 7a3ece4 to 55d7d05 Compare September 15, 2020 20:21
@azhogov azhogov merged commit d590144 into openvinotoolkit:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: IE Tests OpenVINO Test: plugins and common category: preprocessing (deprecated) Inference Engine Preprocessing library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants