[Opset13][FP8] Introduce FakeConvert op core #20930

mitruska · 2023-11-07T13:36:55Z

Details:

Introduction of FakeConvert op shell for FP8 types
Based on: POC by @andreyanufr

The following parts will be introduced as a separate PRs:

support for evaluate method and reference implementation
shape inference and input validation improvements
MO IR Reader
Python API
op specification

Tickets:

19976

src/core/src/op/fake_convert.cpp

mitruska · 2023-11-07T14:46:09Z

src/core/include/openvino/op/fake_convert.hpp

+    FakeConvert(const ov::Output<ov::Node>& arg,
+                const ov::Output<ov::Node>& scale,
+                const ov::Output<ov::Node>& shift,
+                const std::string& destination_type = "F8E4M3",
+                bool apply_scale = true);


Hi @andreyanufr @AlexKoff88, I wonder can we drop the apply_scale attribute in favour of having two constructors and then apply_scale if the scale and shift are provided, in other words if (inputs.size() == 3). Do you see a need to keep this attribute and switch off the scaling even if the inputs are provided?

Suggested change

FakeConvert(const ov::Output<ov::Node>& arg,

const ov::Output<ov::Node>& scale,

const ov::Output<ov::Node>& shift,

const std::string& destination_type = "F8E4M3",

bool apply_scale = true);

FakeConvert(const ov::Output<ov::Node>& arg,

const std::string& destination_type = "F8E4M3");

FakeConvert(const ov::Output<ov::Node>& arg,

const ov::Output<ov::Node>& scale,

const ov::Output<ov::Node>& shift,

const std::string& destination_type = "F8E4M3");

apply_scale was added for research purposes only. This should not be in the current implementation.

@andreyanufr, do we really need apply_scale at all?

@AlexKoff88 , we don't need it. I think we will put scale and shift outside of this operation in the network graph.

As discussed it's better to keep scaling within the FakeConvert logic, in the updated version scale input is required and shift input optional.
The apply_scale attribute has been removed.

mitruska · 2023-11-07T14:49:43Z

src/core/include/openvino/op/fake_convert.hpp

+
+private:
+    void validate() const;
+    std::string m_destination_type = "F8E4M3";


When the FP8 types will be a part of the ov::element, this destination type should be ov::element, for now it can be string as proposed in the POC (or improved to be struct/enum).

src/core/src/op/fake_convert.cpp

AlexKoff88 · 2023-11-08T10:16:10Z

src/core/include/openvino/op/fake_convert.hpp

+    FakeConvert(const ov::Output<ov::Node>& arg,
+                const ov::Output<ov::Node>& scale,
+                const ov::Output<ov::Node>& shift,
+                const std::string& destination_type = "F8E4M3",


Should we change F8E4M3 to f8e4m3 per our agreement? Is it consistent with the names of other data types?

I will update it to lowercase, in case of keeping it as string, I thought that both options should be allowed (upper and lower case) and unified with std::tolower. We can stick with lower case only.
Currently for ov::element upper and lower case are acceptable:

openvino/src/core/src/type/element_type.cpp

Lines 80 to 82 in d07f272

if (type == "f16" || type == "FP16") {

return ::ov::element::Type(::ov::element::Type_t::f16);

} else if (type == "f32" || type == "FP32") {

Is it okay to keep the destination_type as string (at least for now, and update it to ov::element later) or it's better to introduce temporary enum representation for FP8 types attribute?

I think it is ok.

…vert_core_op

src/core/src/op/fake_convert.cpp

…vert_core_op

andreyanufr · 2023-11-08T09:35:43Z

src/core/include/openvino/op/fake_convert.hpp

+    FakeConvert(const ov::Output<ov::Node>& arg,
+                const ov::Output<ov::Node>& scale,
+                const ov::Output<ov::Node>& shift,
+                const std::string& destination_type = "F8E4M3",
+                bool apply_scale = true);


apply_scale was added for research purposes only. This should not be in the current implementation.

andreyanufr · 2023-11-08T10:17:52Z

src/core/include/openvino/op/fake_convert.hpp

+    FakeConvert(const ov::Output<ov::Node>& arg,
+                const ov::Output<ov::Node>& scale,
+                const ov::Output<ov::Node>& shift,
+                const std::string& destination_type = "F8E4M3",
+                bool apply_scale = true);


@AlexKoff88 , we don't need it. I think we will put scale and shift outside of this operation in the network graph.

AlexKoff88 · 2023-11-13T13:22:00Z

@AlexKoff88 , we don't need it. I think we will put scale and shift outside of this operation in the network graph.

I don't think we should keep scale and shift outside of operation. Otherwise, it is gonna be long subgraphs again, such as Multiply(s)->Subtract(zp)->FakeConvert->Add(zp)->Multiply(1/s) for each operation and its weights. This will blow off the model significantly and lead to much larger representation, longer transformations, more complicated model processing, etc. It can hurt LLMs.

slyalin · 2023-11-13T13:32:20Z

src/core/src/op/fake_convert.cpp

+
+std::shared_ptr<ov::Node> FakeConvert::clone_with_new_inputs(const ov::OutputVector& new_args) const {
+    OV_OP_SCOPE(v13_FakeConvert_clone_with_new_inputs);
+    OPENVINO_ASSERT(new_args.size() == 3, "Incorrect number of new arguments");


As scale and shift are not mandatory, please make them optional. Otherwise I don't understand why we have apply_scale, but don't have apply_shift.

If you make both scale and shift optional, you really need to have only apply_scale among attributes. Otherwise apply_shift is required.

Let's make the shift an optional parameter. I would make scale as required.

I think that we always need to apply scale, but shift can be optional.

As discussed, in the updated version scale input is required and shift input optional.
The apply_scale attribute has been removed.

slyalin · 2023-11-13T13:54:35Z

src/core/include/openvino/op/fake_convert.hpp

+                const ov::Output<ov::Node>& scale,
+                const ov::Output<ov::Node>& shift,
+                std::string destination_type = "f8e4m3",
+                bool apply_scale = false);


No reason to have apply_scale set to false by default, suggesting to set it automatically in case if scale input is provided (and sale and shift inputs are made optional as suggested in another comment). If one wants to provide shift but doesn't apply scale, then as we don't have "gaps" in the list of operation arguments, one should provide arbitrary scale (will be ignored), then shift and set apply_scale to false. This is only (and looks like very rare situation) when apply_scale should have manual control as a parameter of constructor.

FakeConvert(input) ==> apply_scale = false (as no scale provided)
FakeConvert(input, scale) ==> apply_scale = true
FakeConvert(input, scale, shift) ==> apply_scale = true
FakeConvert(input, scale, shift, "f8e4m", false) ==> apply_scale = false, scale input is ignored

That's why I started the conversation above: #20930 (comment)
So the final suggestion is to have three constructors and the apply_scale attribute.

As discussed, in the updated version scale input is required and shift input optional.
The apply_scale attribute has been removed.

slyalin

Summary: change scale and shift to optional, rework ctor signature to be more practical.

…vert_core_op

mitruska · 2023-11-13T20:56:11Z

@slyalin, @AlexKoff88, @andreyanufr As discussed offline, removed apply_scale attribute, keeping scale as required input and shift as optional.
Please re-review.

slyalin · 2023-11-14T14:32:09Z

src/core/src/op/fake_convert.cpp

+        return std::make_shared<FakeConvert>(new_args.at(0), new_args.at(1), m_destination_type);
+    } else if (new_args.size() == 3) {
+        return std::make_shared<FakeConvert>(new_args.at(0), new_args.at(1), new_args.at(2), m_destination_type);


@mitruska, let me put it here but it is not a blocker for this PR, just a general comment for Op enabling that can be made simpler: clone_with_new_inputs can be generated automatically with using default ctor that is available for all operations, and the visitors. It is applicable for all the operations and utilizing such a mechanism will simplify the process of new ops enabling. I've triggered on this condition that just a consequence of having two ctors with a different number of arguments. Various ctors are provided for externals op usage for convenience in C++, but for such a standard procedure as cloning, it makes sense to use more low-level approach. Compare with serialization/deserialization: they are implemented automatically using the same low-level apparatus.

slyalin · 2023-11-15T13:06:15Z

src/core/include/openvino/op/fake_convert.hpp

+private:
+    void validate_type() const;
+
+    std::string m_destination_type = "f8e4m3";


Why is this string? Should be ov::element::Type. See Convert class for reference -- no reason to have an alternative way to describe the type.
Sorry, missed this when reviewing because didn't really expect a trap here.

Requires adding new types here: https://github.com/openvinotoolkit/openvino/blob/master/src/core/include/openvino/core/type/element_type.hpp#L35-L55

* FakeConvert op init * Update dest types names * Update op hpp * Update opset ops number * Init type_prop tests * Add attributes tests * Add op check test * Update namespace in fc cpp * Update getters * Refactor static member * Make destination_type lower case * Update type in test * Move get_valid_types out of class * Update ops number in opset * Remove apply_scale attribute * Additional constructor to make `shift` input optional

mitruska added 9 commits November 6, 2023 22:22

FakeConvert op init

82649ed

Update dest types names

e34026b

Update op hpp

c75b12e

Update opset ops number

1ff36f3

Init type_prop tests

ccc4c86

Add attributes tests

b064102

Add op check test

a7aa173

Update namespace in fc cpp

e39f16f

Update getters

1430dc9

github-actions bot added category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common category: CPP API OpenVINO CPP API bindings labels Nov 7, 2023

ilya-lavrenov reviewed Nov 7, 2023

View reviewed changes

src/core/src/op/fake_convert.cpp Outdated Show resolved Hide resolved

mitruska commented Nov 7, 2023

View reviewed changes

mitruska marked this pull request as ready for review November 7, 2023 15:36

mitruska requested review from a team as code owners November 7, 2023 15:36

AlexKoff88 requested a review from slyalin November 7, 2023 15:49

mitruska requested review from AlexKoff88, praasz and andreyanufr November 8, 2023 09:11

AlexKoff88 reviewed Nov 8, 2023

View reviewed changes

mitruska added 3 commits November 8, 2023 13:58

Refactor static member

f444057

Make destination_type lower case

b821657

Merge remote-tracking branch 'upstream/master' into mitruska/fake_con…

be23a19

…vert_core_op

ilya-lavrenov reviewed Nov 8, 2023

View reviewed changes

src/core/src/op/fake_convert.cpp Outdated Show resolved Hide resolved

mitruska added 2 commits November 8, 2023 16:11

Update type in test

849c410

Move get_valid_types out of class

9474401

AlexKoff88 approved these changes Nov 8, 2023

View reviewed changes

mitruska self-assigned this Nov 8, 2023

mitruska added this to the 2023.3 milestone Nov 8, 2023

mitruska added the category: Opset OpenVINO Opset label Nov 8, 2023

mitruska added 3 commits November 9, 2023 13:23

Merge remote-tracking branch 'upstream/master' into mitruska/fake_con…

22160f6

…vert_core_op

Update ops number in opset

821f5f4

Merge branch 'master' into mitruska/fake_convert_core_op

e36c41b

andreyanufr approved these changes Nov 13, 2023

View reviewed changes

slyalin reviewed Nov 13, 2023

View reviewed changes

slyalin requested changes Nov 13, 2023

View reviewed changes

mitruska added 3 commits November 13, 2023 20:58

Remove apply_scale attribute

213f45e

Additional constructor to make shift input optional

a51f998

Merge remote-tracking branch 'upstream/master' into mitruska/fake_con…

42e67aa

…vert_core_op

mitruska requested review from slyalin, andreyanufr and AlexKoff88 November 13, 2023 20:53

AlexKoff88 approved these changes Nov 14, 2023

View reviewed changes

andreyanufr approved these changes Nov 14, 2023

View reviewed changes

slyalin reviewed Nov 14, 2023

View reviewed changes

slyalin approved these changes Nov 14, 2023

View reviewed changes

Merge branch 'master' into mitruska/fake_convert_core_op

04d706e

mitruska enabled auto-merge (squash) November 14, 2023 19:54

mitruska merged commit afc5995 into openvinotoolkit:master Nov 14, 2023
35 checks passed

slyalin reviewed Nov 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Opset13][FP8] Introduce FakeConvert op core #20930

[Opset13][FP8] Introduce FakeConvert op core #20930

mitruska commented Nov 7, 2023 •

edited

Loading

mitruska Nov 7, 2023

andreyanufr Nov 8, 2023

AlexKoff88 Nov 8, 2023

andreyanufr Nov 8, 2023

mitruska Nov 13, 2023

mitruska Nov 7, 2023

AlexKoff88 Nov 8, 2023

mitruska Nov 8, 2023

AlexKoff88 Nov 13, 2023

andreyanufr Nov 8, 2023

andreyanufr Nov 8, 2023

AlexKoff88 commented Nov 13, 2023

slyalin Nov 13, 2023

slyalin Nov 13, 2023

AlexKoff88 Nov 13, 2023

andreyanufr Nov 13, 2023

mitruska Nov 13, 2023

slyalin Nov 13, 2023 •

edited

Loading

slyalin Nov 13, 2023 •

edited

Loading

mitruska Nov 13, 2023

mitruska Nov 13, 2023

slyalin left a comment

mitruska commented Nov 13, 2023

slyalin Nov 14, 2023

slyalin Nov 15, 2023

slyalin Nov 15, 2023

	if (type == "f16" \|\| type == "FP16") {
	return ::ov::element::Type(::ov::element::Type_t::f16);
	} else if (type == "f32" \|\| type == "FP32") {

[Opset13][FP8] Introduce FakeConvert op core #20930

[Opset13][FP8] Introduce FakeConvert op core #20930

Conversation

mitruska commented Nov 7, 2023 • edited Loading

Details:

Tickets:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKoff88 commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyalin Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

slyalin Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyalin left a comment

Choose a reason for hiding this comment

mitruska commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitruska commented Nov 7, 2023 •

edited

Loading

slyalin Nov 13, 2023 •

edited

Loading

slyalin Nov 13, 2023 •

edited

Loading