Dimitris/requantize bmm ref by DimitrisPapac · Pull Request #78 · NP-Eng/verifiaml

DimitrisPapac · 2024-04-26T09:27:03Z

In summary, this pull request adds new nodes implementing TF Lite's reference requantization method, as well as a simplified (i.e., single-round) requantization variant. This involved some drastic refactoring throughout the codebase in order to accommodate the newly added nodes and use more meaningful identifiers wherever possible (for example, Qarray to Tensor, InnerType to Integral, etc.) These changes resulted in significant code cleanup. A number of tests are also included. This pull request subsumes pull requests: #70 and #73 .

Co-authored-by: Cesar Descalzo <Cesar199999@users.noreply.github.com>

…bmm-ref

…_multiplier.

…ion node generic without floating the type all the way to the model Co-authored-by: Antonio95 <anmegi.95@gmail.com>

…uantisation node generic without floating the type all the way to the model" This reverts commit 75caebd8cbeb809b197c53087f69892fa49785b6.

Co-authored-by: Antonio95 <anmegi.95@gmail.com>

…ation

Co-authored-by: Antonio95 <anmegi.95@gmail.com>

…innertype Various refactors around `QArray` and requantisation

…large-types

Co-authored-by: Antonio Mejías Gil <anmegi.95@gmail.com> Signed-off-by: Cesar Descalzo <80001463+Cesar199999@users.noreply.github.com>

…ge-types Replace QTypeArray by NIOTensor

Antonio95

Very good job! The thorough tests are great. There are no meaningful changes to make as far as I am concerned: I have simply left a couple of tiny comments.

common/src/model/nodes/mod.rs

common/src/model/nodes/requantize_bmm_ref.rs

common/src/model/nodes/requantize_bmm_single.rs

common/src/quantization/mod.rs

common/src/model/nodes/requantize_bmm_ref.rs

Cesar199999

Nice! Thanks a lot @DimitrisPapac for this great PR :)

I already had the chance to review #70 before it was merged here. Furthermore, while working on #73 (merged here) and #67 (ongoing in antonio-cesar/trait-objects) I could see that everything works. There's not much to add from my side, appart from a couple of design question (that could be done as part of #76 and #75 in future PRs).

common/src/quantization/tests.rs

mmagician

non-comprehensive review. overall looks good, left some minor comments

common/src/model/nodes/mod.rs

mmagician · 2024-04-29T19:23:50Z

Cargo.toml

 blake2 = { version = "0.10", default-features = false }
 serde_json = "1.0.108"
-ark-pcs-bench-templates = { git = "https://github.com/HungryCatsStudio/poly-commit", branch = "ligero-uni-and-ml-absorb", default-features = false }
+ark-pcs-bench-templates = { git = "https://github.com/HungryCatsStudio/poly-commit", rev = "dfdd8e8" }


this revision is already in master, and I think we should stick to revs no?

Hey @mmagician, I think you're right. In any case, you can regard the Cargo.toml here as a temporary thing: the one in main was recently cleaned up, pinned to specific revs and made much more consistent (by @Cesar199999, I think).

Things will be much cleaner when main is merged into this PR (which, if I recall correctly, @Cesar199999 will take care of?). In principle that will happen very soon, when the remaining tiny open discussions are concluded.

Yes, on it!

mmagician · 2024-04-29T19:27:01Z

common/src/compatibility/example_models/two_layer_perceptron_mnist/mod.rs

+    let req_bmm_2 = match req_strategy {
+        BMMRequantizationStrategy::Floating => Node::RequantizeBMMFloat(
+            RequantizeBMMFloatNode::new(OUTPUT_DIM, S_2_I, Z_2_I, S_2_W, Z_2_W, S_2_O, Z_2_O),
+        ),
+        BMMRequantizationStrategy::Reference => Node::RequantizeBMMRef(RequantizeBMMRefNode::new(
+            OUTPUT_DIM, S_2_I, S_2_W, S_2_O, Z_2_O,
+        )),
+        BMMRequantizationStrategy::SingleRound => Node::RequantizeBMMSingle(
+            RequantizeBMMSingleNode::new(OUTPUT_DIM, S_2_I, S_2_W, S_2_O, Z_2_O),
+        ),
+    };


can this be simplified? e.g. with a macro

Indeed! I went for a function, since it seemed quite natural. Already pushed to the PR branch. Diff here: https://github.com/HungryCatsStudio/verifiaml/compare/f91ecadc5eea54bc532c3119a4906dfb13cc5d88~1..f91ecadc5eea54bc532c3119a4906dfb13cc5d88

This is a good cleanup, thanks for the suggestion

mmagician · 2024-04-29T19:30:35Z

common/src/compatibility/example_models/simple_perceptron_mnist/mod.rs

 // TODO this is incorrect now that we have switched to logs
-pub fn build_simple_perceptron_mnist<F, S, PCS>() -> Model<i8, i32>
+pub fn build_simple_perceptron_mnist<F, S, PCS>(
+    req_strategy: BMMRequantizationStrategy,


so this means there's one requantization strategy per model, yes?
Is this what we want?

Yes, basically these are just auxiliary functions that construct our example models by filling the vector of nodes with the nodes it should have. In one of the models there is only one requantisation node and in the other one there are two. In the latter case, we don't have any need to mix requantisation strategies (which sounds a bit unlikely anyway). Still, it should be stressed that this is just code to build examples, not library functionality code. Therefore the "lack of generality" shouldn't be a problem.

Incidentally, the reason we added this argument was so that we could test out how much the reference, single-round and floating-point-based implementations of requantisation each differ from TF Lite execution.

mmagician · 2024-04-29T19:31:48Z

common/src/model/nodes/mod.rs

+    RequantizeBMMFloat(RequantizeBMMFloatNode<ST>),
+    RequantizeBMMRef(RequantizeBMMRefNode<ST>),
+    RequantizeBMMSingle(RequantizeBMMSingleNode<ST>),


Ok def need better names :)

Hey @mmagician, what bothers you exactly? Is it how long these lines end up being?
I think the current naming is at least consistent: for each implementor <name>Node of the NodeOps... trait, we have one variant <name>(<name>Node) of the Node enum. Furthermore, as we also discussed a while ago, these requantisation nodes are specific to BMM outputs (e.g. the output of a convolutional layer will likely have a slightly different requantisation code - although I'm not sure tbh; if that turns out not to be the case, we could remove BMM from all of these).

Do you have some better name for these in mind? If it is the length that bothers you, I would propose shortening Requantize to Req:

ReqBMMFloat(ReqBMMFloatNode<ST>), ReqBMMRef(ReqBMMRefNode<ST>), ReqBMMSingle(ReqBMMSingleNode<ST>),

As a side note, this code (and in general variant code pattern <name>(<name>Node)) will go away if/when we switch to trait objects.

…iants

DimitrisPapac and others added 30 commits April 8, 2024 10:02

Added .gitignore

50352aa

Added code for TF Lite's reference implementation

78c80af

Attempted to fix trait bounds

8618e14

Attempted to fix trait bounds.

c5fea81

Co-authored-by: Cesar Descalzo <Cesar199999@users.noreply.github.com>

Collaboratively fixed the code

e8f6154

Fixed bug in frexp

9082cf7

Merge branch 'cesar/speed-up-compatibility' into dimitris/requantize-…

c35f20a

…bmm-ref

Add use_requantise_ref flag in model builders

661c03a

Made fixes to quantize_multiplier. Added tests for frexp and quantize…

93ad93e

…_multiplier.

Small code changes

e38cf8a

Add requantisation reference compatibility tests

73aba23

removed hardcoded types QScaleType, etc.; cannot make BMM requantisat…

30b0e12

…ion node generic without floating the type all the way to the model Co-authored-by: Antonio95 <anmegi.95@gmail.com>

Revert "removed hardcoded types QScaleType, etc.; cannot make BMM req…

39b9f70

…uantisation node generic without floating the type all the way to the model" This reverts commit 75caebd8cbeb809b197c53087f69892fa49785b6.

Fixed tests and replaced panics with asserts.

15929af

Refactored model constructors

0b0d55d

Co-authored-by: Antonio95 <anmegi.95@gmail.com>

Adapted compatibility tests

c564446

Fixed tests for BMMRef implementation

5aaee3f

Introduced pow2_double in order to fix overflow in requantise_ref.

a6294e4

Debugged requantise_ref

4428263

Co-authored-by: Antonio95 <anmegi.95@gmail.com>

Renamed RequantiseBMMNode to RequantiseBMMFloatNode

19234a9

Increased NB_OUTPUTS to 10000.

c92a115

Completed initial draft of simplified requantization

a71b5e1

Updated simple and two-layer perceptron code for simplified requantiz…

c707f44

…ation

Collaboratively debugged BMM simplified implementation

467829c

Co-authored-by: Antonio95 <anmegi.95@gmail.com>

Updated compatibility tests for the simplified requantization

fc267a6

Added tests and partially fixed simplified requantization.

6d7bb9e

pinned dependencies as in main

1b99e6e

added missing parameter in old function call

de213f5

tweaked names and comments

cfe5115

fixed simplified requantisation

238e1d5

Antonio95 and others added 10 commits April 18, 2024 15:11

switched implementation of Integral to a macro

fb5f32a

removed unecessary (for now) commented-out code

10e7acd

applied consistent use of parentheses

8718a79

Merge pull request #70 from HungryCatsStudio/antonio/refactor_qarray_…

d218dc6

…innertype Various refactors around `QArray` and requantisation

Replace QTypeArray by NIOTensor

d73633a

Merge branch 'dimitris/requantize-bmm-ref' into cesar/bind-small-and-…

3f0c62e

…large-types

Use Into and TryInto instead of From and TryFrom

0b94605

Update common/src/quantization/mod.rs

96c004d

Co-authored-by: Antonio Mejías Gil <anmegi.95@gmail.com> Signed-off-by: Cesar Descalzo <80001463+Cesar199999@users.noreply.github.com>

Update common/src/model/tensor/mod.rs

2ac39f6

Co-authored-by: Antonio Mejías Gil <anmegi.95@gmail.com> Signed-off-by: Cesar Descalzo <80001463+Cesar199999@users.noreply.github.com>

Merge pull request #73 from HungryCatsStudio/cesar/bind-small-and-lar…

a9070de

…ge-types Replace QTypeArray by NIOTensor

DimitrisPapac requested a review from a team as a code owner April 26, 2024 09:27

DimitrisPapac requested review from Antonio95, Cesar199999 and mmagician April 26, 2024 09:27

Antonio95 approved these changes Apr 29, 2024

View reviewed changes

Cesar199999 reviewed Apr 29, 2024

View reviewed changes

common/src/model/nodes/requantize_bmm_ref.rs Show resolved Hide resolved

Cesar199999 approved these changes Apr 29, 2024

View reviewed changes

Cesar199999 reviewed Apr 29, 2024

View reviewed changes

common/src/quantization/tests.rs Show resolved Hide resolved

mmagician reviewed Apr 29, 2024

View reviewed changes

mmagician mentioned this pull request Apr 29, 2024

Introduce trait objects in the codebase #79

Draft

DimitrisPapac and others added 10 commits April 30, 2024 11:20

Addressed part of the comments that were suggested.

07ef113

Updated strings in requantize_bmm_ref.rs and requantize_bmm_single.rs

b05c109

Updated one of the tests in order to match the particular error message.

f63a5a1

minor str fixes

50fa7b9

introduced convenience function to create suitable requantisation var…

f91ecad

…iants

minor message tweak

3b8ac19

Add non-conflicting merge files

a22823c

Remove duplicated dependency

e69ea18

Resolve merge conflicts

454f18d

Rename missing instances of QArray to Tensor

6d60ac4

Conversation

DimitrisPapac commented Apr 26, 2024

Uh oh!

Antonio95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cesar199999 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants