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

[PT FE] [ONNX FE] Partially upcast random_normal to f32 #21400

Merged
merged 28 commits into from
Dec 7, 2023

Conversation

pavel-esir
Copy link
Contributor

@pavel-esir pavel-esir commented Nov 30, 2023

Details:

Tickets:

@github-actions github-actions bot added the category: PyTorch FE OpenVINO PyTorch Frontend label Nov 30, 2023
@pavel-esir pavel-esir force-pushed the upcast_randn_to_f32 branch 2 times, most recently from 5e6eda3 to ed857e9 Compare November 30, 2023 18:15
@github-actions github-actions bot added the category: ONNX FE OpenVINO ONNX FrontEnd label Nov 30, 2023
@pavel-esir pavel-esir changed the title [WIP] upcast randn to f32 [PT FE] [ONNX FE] Upcast random_normal to f32 Nov 30, 2023
@pavel-esir pavel-esir requested review from mvafin and eaidova November 30, 2023 18:18
@pavel-esir pavel-esir added this to the 2023.3 milestone Nov 30, 2023
@pavel-esir pavel-esir added the bug Something isn't working label Nov 30, 2023
@pavel-esir pavel-esir marked this pull request as ready for review November 30, 2023 18:18
@pavel-esir pavel-esir requested review from a team as code owners November 30, 2023 18:18
@pavel-esir pavel-esir changed the title [PT FE] [ONNX FE] Upcast random_normal to f32 [PT FE] [ONNX FE] Upcast partially random_normal to f32 Nov 30, 2023
@pavel-esir pavel-esir changed the title [PT FE] [ONNX FE] Upcast partially random_normal to f32 [PT FE] [ONNX FE] Partially upcast random_normal to f32 Nov 30, 2023
@mvafin
Copy link
Contributor

mvafin commented Nov 30, 2023

Could you add test for this?

@pavel-esir
Copy link
Contributor Author

Some existing tests are failing. Wil fix this

@andrei-kochin
Copy link
Contributor

@pavel-esir should we trigger any performance validation for this to check the potential regression?

@pavel-esir
Copy link
Contributor Author

pavel-esir commented Dec 1, 2023

@pavel-esir should we trigger any performance validation for this to check the potential regression?

I think it's not necessary. These changes are very safe. RandomUniform and Logs are very computationally lightweight, compared to Convolutions, MatMuls. I took a look to execution graph and Log/RandomUniform both take ~2 microsecs, while Conv ~320 microsecs.

@pavel-esir pavel-esir requested a review from a team as a code owner December 5, 2023 16:25
@pavel-esir pavel-esir requested review from a team as code owners December 6, 2023 13:55
@github-actions github-actions bot added category: CPU OpenVINO CPU plugin category: CPP API OpenVINO CPP API bindings labels Dec 6, 2023
@ilya-lavrenov ilya-lavrenov self-requested a review December 6, 2023 14:43
@github-actions github-actions bot removed the category: CPU OpenVINO CPU plugin label Dec 6, 2023

#pragma once

#include "ngraph/output_vector.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

please, avoid use of legacy API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix this in a separate PR

@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Dec 7, 2023
@ilya-lavrenov ilya-lavrenov self-requested a review December 7, 2023 07:21
@ilya-lavrenov ilya-lavrenov enabled auto-merge (squash) December 7, 2023 09:34
@ilya-lavrenov ilya-lavrenov dismissed vurusovs’s stale review December 7, 2023 11:23

will be fixed separately

@ilya-lavrenov ilya-lavrenov merged commit b71906c into openvinotoolkit:master Dec 7, 2023
73 checks passed
@pavel-esir pavel-esir deleted the upcast_randn_to_f32 branch December 7, 2023 11:24
akuporos pushed a commit to akuporos/openvino that referenced this pull request Dec 8, 2023
…kit#21400)

* upcast randn to fp32

* style fix

* corrected tests

* add layer tests with statistics

* style-fix

* move make_random_normal to cmmmon

* style-fix

* added randn layer tests; updated CMakeLists.txt

* moved to inline

* fix problem with _USE_MATH_DEFINES on Win

* pass NodeRegistry as reference; some other minor corrections

* adjust thresholds to avoid sporadicity

* move random_normal_helper and hide from public api

* fix install

* fix install: 2nd try

* Frontend common

* remove last frontend_common::static

* build fix

* try to fix mock1 build: 2nd attempt

* try to fix mock1 build: 3rd attempt

* Update src/core/tests/CMakeLists.txt

* Fixed build: attemp 2

* Update src/plugins/intel_cpu/tests/unit/CMakeLists.txt

* Update CMakeLists.txt

---------

Co-authored-by: Ilya Lavrenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working category: build OpenVINO cmake script / infra category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: IE Tests OpenVINO Test: plugins and common category: inference OpenVINO Runtime library - Inference category: ONNX FE OpenVINO ONNX FrontEnd category: PyTorch FE OpenVINO PyTorch Frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants