Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FC+elementwise_add (residual connection) #41776

Merged
merged 53 commits into from
Apr 14, 2022

Conversation

Silv3S
Copy link
Member

@Silv3S Silv3S commented Apr 13, 2022

PR types

Performance optimization

PR changes

OPs

Describe

  • Fuse FC with elementwise_add so residual data is added inside FC operator,
  • add unit tests with multiple cases for this fuse pass,
  • download VIT-OCR model and perform accuracy test.

Benchmarks

Test were conducted with oneDNN 2.5.4 , fc_mkldnn_pass and fc_act_mkldnn_pass were enabled:

  • VIT-OCR is performing 4% faster (12 fc+residual fusions),
  • BERT is performing 8% faster (24 fc+residual fusions).

@paddle-bot-old paddle-bot-old bot added contributor External developers status: proposed labels Apr 13, 2022
@paddle-bot-old
Copy link

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Copy link
Contributor

@zhiqiu zhiqiu left a comment

Choose a reason for hiding this comment

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

LGTM

@Silv3S
Copy link
Member Author

Silv3S commented Apr 14, 2022

Hi @XieYunshen @chalsliu could you please approve setting TIMEOUT for newly added unit test?

Copy link
Contributor

@XieYunshen XieYunshen 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 set_tests_properties(test_mkldnn_fc_elementwise_add_fuse_pass PROPERTIES TIMEOUT 120)

@lidanqing-intel lidanqing-intel merged commit 92d8d0b into PaddlePaddle:develop Apr 14, 2022
@paddle-bot-old
Copy link

你的PR已合入Paddle库,请关注后续测试结果。
Your PR has been merged into the repository. An official integration test will be conducted later. Stay tuned.

@lidanqing-intel
Copy link
Contributor

#40834 has been reviewed by many developers and performance and model UT was given. This PR is the same as #40834. Hence I merge it.

@Silv3S Silv3S deleted the residual_ci branch April 14, 2022 11:21
@baoachun
Copy link
Contributor

baoachun commented Apr 15, 2022

Hi @Silv3S, this pr will cause the test_analyzer_vit_ocr single test execution of the Windows task to fail, could you plsease check it? https://xly.bce.baidu.com/paddlepaddle/paddle/newipipe/detail/5400245/job/14068509

endif()
inference_analysis_test(test_analyzer_vit_ocr SRCS analyzer_vit_ocr_tester.cc
EXTRA_DEPS ${INFERENCE_EXTRA_DEPS}
ARGS --infer_model=${VIT_OCR_INSTALL_DIR}/vit_ocr --infer_data=${VIT_OCR_INSTALL_DIR}/datavit.txt)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Silv3S Hi, this new test failed on windows, please investigate thank you.

@Silv3S
Copy link
Member Author

Silv3S commented Apr 18, 2022

@baoachun @lidanqing-intel Thank you for letting me know. As I can see there is error with downloading model and data for test. That's unexpected because all windows tests were ok on PR CI and test_analyzer_vit_ocr works locally for me. I will investigate more and fix it

@lidanqing-intel
Copy link
Contributor

lidanqing-intel commented Apr 20, 2022

Hi, @baoachun . Siwek checked it seems models downloading issue and the link address is rarely used in API tests before. Please move vit_ocr.tgz and datavit.txt to more frequently used link and combine them into one tgz.

original model link
https://paddle-qa.bj.bcebos.com/inference_model/2.1.1/ocr/vit_ocr.tgz
original data link
https://paddle-qa.bj.bcebos.com/inference_model/2.1.1/ocr/datavit.txt

Move the two files into one http://paddle-inference-dist.bj.bcebos.com/ocr/vit_ocr.tgz.
Its distracted files should look like

- vit_ocr/
  - datavit.txt
  - model/
    - inference.pdiparams
    - inference.pdmodel  

Thanks.

@Silv3S
Copy link
Member Author

Silv3S commented Apr 20, 2022

I changed config, and now VIT-OCR model and data will be downloaded from same source as other models #42041

@paddle-bot-old paddle-bot-old bot removed the contributor External developers label Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants