Skip to content

Capi: Ngraph engine#17037

Merged
luotao1 merged 1 commit intoPaddlePaddle:developfrom
mozga-intel:mozga-intel/capi_ngraph
May 29, 2019
Merged

Capi: Ngraph engine#17037
luotao1 merged 1 commit intoPaddlePaddle:developfrom
mozga-intel:mozga-intel/capi_ngraph

Conversation

@mozga-intel
Copy link
Copy Markdown
Contributor

@mozga-intel mozga-intel commented Apr 22, 2019

Enable the capi pass for the nGraph bridge. BERT for an inference.

@mozga-intel mozga-intel requested a review from luotao1 April 22, 2019 16:35
@mozga-intel mozga-intel changed the title The capi version of code for a Ngraph engine Capi: Ngraph engine Apr 22, 2019
@mozga-intel mozga-intel force-pushed the mozga-intel/capi_ngraph branch from 245673c to 210a017 Compare April 22, 2019 17:26
@luotao1 luotao1 requested a review from tensor-tang April 23, 2019 02:03
@mozga-intel mozga-intel force-pushed the mozga-intel/capi_ngraph branch 4 times, most recently from 27e8f30 to 99e4aa4 Compare May 9, 2019 08:04
@mozga-intel
Copy link
Copy Markdown
Contributor Author

@tensor-tang @luotao1 Can you look at this PR?

@mozga-intel
Copy link
Copy Markdown
Contributor Author

@tensor-tang @luotao1 Can you look at this PR, this pull-request is needed to support BERT model?

@mozga-intel mozga-intel added this to the v1.5 for Intel milestone May 14, 2019
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.

Why not use

pass_library(ngraph_subgraph_pass inference ngraph)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, your suggestion seems to be fine, Can I take this suggestion into consideration, and do it in the next pull-request?

Copy link
Copy Markdown
Contributor

@baojun-nervana baojun-nervana May 22, 2019

Choose a reason for hiding this comment

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

it seems it won't work due to some dependency

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.

Can I take this suggestion into consideration, and do it in the next pull-request?

Yes, please do it in the next PR.

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.

Here is cfg. EnableNgraph();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Why do you update batch_norm_op.h and conv2d_op.h here? They are not related to this PR.

Copy link
Copy Markdown
Contributor Author

@mozga-intel mozga-intel May 21, 2019

Choose a reason for hiding this comment

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

On the whole, these changes are necessary for the CAPI tests - to pass the tests. I agree with you that it is not associated with this PR, but these changes are really small. If you want I can move it to separate PR - but it will generate additional time.

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.

depthwise_conv can be done in a separate PR (#17454). The batchnorm update is to pass the test due to the BN fast math issue. That is the easiest way to make it align with paddle precision.

@luotao1
Copy link
Copy Markdown
Contributor

luotao1 commented May 21, 2019

@Superjomn Could you help review inference api related interface: paddle_analysis_config.h, analysis_config.cc?

Comment thread paddle/fluid/framework/ir/ngraph_subgraph_pass.cc Outdated
Comment thread paddle/fluid/framework/ir/ngraph_subgraph_pass.cc Outdated
Copy link
Copy Markdown
Contributor

@tensor-tang tensor-tang left a comment

Choose a reason for hiding this comment

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

OK thanks @mozga-intel @baojun-nervana .
Please fix the conflicts

@mozga-intel mozga-intel force-pushed the mozga-intel/capi_ngraph branch from cf239f8 to 3e2ff53 Compare May 28, 2019 17:01
@mozga-intel
Copy link
Copy Markdown
Contributor Author

@tensor-tang @luotao1 Thanks, The conflicts are resolved.

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.

Can I take this suggestion into consideration, and do it in the next pull-request?

Yes, please do it in the next PR.

@luotao1 luotao1 merged commit 5eb81fe into PaddlePaddle:develop May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants