chore: implement hybrid model demo with GPT-2 #246

fd0r · 2023-09-13T08:28:32Z

Closes https://github.com/zama-ai/concrete-ml-internal/issues/3842
Closes https://github.com/zama-ai/concrete-ml-internal/issues/3869
Closes https://github.com/zama-ai/concrete-ml-internal/issues/3875
Closes https://github.com/zama-ai/concrete-ml-internal/issues/3855
Closes https://github.com/zama-ai/concrete-ml-internal/issues/3852

andrei-stoian-zama · 2023-09-15T08:58:16Z

Closes

fd0r · 2023-09-15T10:37:29Z

Yes I think so.
Currently fixing the batch-size/gemm optimization issue.

jfrery

Nicely done! I have a few comments as well as question for my understanding.

use_case_examples/llm/QGPT2Evaluate.ipynb

src/concrete/ml/torch/compile.py

src/concrete/ml/torch/hybrid_model.py

tests/torch/test_compile_torch.py

use_case_examples/hybrid_model/README.md

use_case_examples/hybrid_model/compile_hybrid_llm.py

use_case_examples/hybrid_model/load_and_analyze_data.py

jfrery · 2023-09-19T07:39:22Z

use_case_examples/llm/QGPT2Evaluate.ipynb

@@ -695,5 +695,5 @@
  }
 },
 "nbformat": 4,
- "nbformat_minor": 2
+ "nbformat_minor": 4


Is this expected?

No idea actually, nbqa probably? 🤷🏼

Not sure. It's not important I guess.

I didn't modify the file myself

Can't do much about this one

probably a version difference or something

jfrery · 2023-09-19T07:58:15Z

src/concrete/ml/torch/hybrid_model.py

Could you add HybridFHEModel to init.py so that we can do from concrete.ml.torch import HybridFHEModel ?

For some reason this result in a circular import

I guess it's because the hybridmodel.py import something from concrete.ml.torch? Not sure there is an easy solution then. We leave it like this?

src/concrete/ml/torch/hybrid_model.py

andrei-stoian-zama

Looks good! Some minor changes especially to docs: ex: better docstrings for the arguments to the Hybrid class

andrei-stoian-zama · 2023-09-13T16:51:39Z

src/concrete/ml/onnx/convert.py

@@ -47,7 +47,58 @@ def get_equivalent_numpy_forward_and_onnx_model(
        opset_version=OPSET_VERSION_FOR_ONNX_EXPORT,
    )
    equivalent_onnx_model = onnx.load_model(str(output_onnx_file_path))
-
+    # List of all currently supported onnx passes


do we keep this comment here or can we link to the list in the onnxoptimizer repo ?

Link should be enough, I'll remove

I removed and kept only the link to the repository

andrei-stoian-zama · 2023-09-19T07:42:15Z

src/concrete/ml/onnx/convert.py

+            continue
+        # Store MatMul node output name
+        matmul_node_output_name = matmul_node.output[0]
+        assert len(matmul_node.output) == 1


good! we expect the matmul node to always have a single output

Yes that is expected, this is more of a guide for someone reading the code than a real assert

andrei-stoian-zama · 2023-09-19T07:44:22Z

src/concrete/ml/onnx/convert.py


 from .onnx_utils import IMPLEMENTED_ONNX_OPS, execute_onnx_with_numpy, get_op_type

 OPSET_VERSION_FOR_ONNX_EXPORT = 14


-def get_equivalent_numpy_forward_and_onnx_model(
+# pylint: disable=too-many-branches
+def fuse_matmul_bias_to_gemm(onnx_model: onnx.ModelProto):


Thanks! 🙏🏼

andrei-stoian-zama · 2023-09-19T07:45:20Z

src/concrete/ml/onnx/convert.py

+    # Optimize ONNX graph
+    # List of all currently supported onnx optimizer passes
+    # From https://github.com/onnx/optimizer/blob/master/onnxoptimizer/pass_registry.h
+    # onnx_passes = [


do we keep this here ?

yes maybe it's not necessary to keep them all here since the relevant link is given

Removed them

src/concrete/ml/torch/compile.py

andrei-stoian-zama · 2023-09-19T08:01:53Z

src/concrete/ml/torch/hybrid_model.py

-        rounding_threshold_bits: int = 8,
-        p_error=0.01,
-        configuration: Configuration = None,
+        rounding_threshold_bits: Optional[int] = 8,


this seems like a weird default to me.. why is 8 a good value ? do we know it's a good value for LLMs or for any NN in general ?

I'll change this to the normal defaults (None) this was here from a previous PR

Changed defaults back to normal

tests/torch/test_compile_torch.py

andrei-stoian-zama · 2023-09-19T08:07:31Z

use_case_examples/hybrid_model/infer_hybrid_llm_generate.py

+    print(f"Using device: {device}")
+
+    # Get GPT2 from Huggingface
+    # TODO: migrate to auto-model with model_name


can you make an issue on this ?

This is done already no? The following lines use the automodel

I removed the comment

andrei-stoian-zama · 2023-09-19T08:15:38Z

use_case_examples/hybrid_model/infer_hybrid_llm_generate.py

+from concrete.ml.torch.hybrid_model import FHEMode, HybridFHEModel
+
+if __name__ == "__main__":
+    configs = [


can you explain what is in this config structure ?

also, they are already defined in the compile file : maybe refactor this into a single config file to avoid any unwanted mismatches ?

I was thinking about it but creating a file just for that seemed a bit overkill.

Can do it if you think it's necessary.
I also thought about dumping a json in the compile file with the configuration that is then re-used in the inference script.
Would that work for you?

I think having a config file is fine but as you wish, second solutions seems ok

in any case as Andrei said a comment explaining what these configs are would be great as well !

Compilation dumps a json that can then be used by the client

use_case_examples/hybrid_model/load_and_analyze_data.py

src/concrete/ml/deployment/fhe_client_server.py

src/concrete/ml/onnx/convert.py

src/concrete/ml/quantization/post_training.py

src/concrete/ml/torch/compile.py

use_case_examples/hybrid_model/compile_hybrid_llm.py

RomanBredehoft · 2023-09-20T15:16:25Z

use_case_examples/hybrid_model/compile_hybrid_llm.py

+        default=["transformer.h.0.attn.c_proj"],
+        type=module_names_parser,
+        help="""The module(s) name(s) to compile to FHE.
+Examples for GPT-2 model:


weird indent, but great thanks !

RomanBredehoft

I would try to see if it's possible to use argparse in serve.py but if not, than looks good to me ! Huge work here thanks a lot for that

jfrery

Looks good to me. I am testing this with phi1.5 to see how it generalizes.

jfrery · 2023-09-21T07:09:54Z

use_case_examples/hybrid_model/requirements.txt

It misses loguru I think?

use_case_examples/hybrid_model/serve_model.py

use_case_examples/hybrid_model/infer_hybrid_llm_generate.py

closes zama-ai/concrete-ml-internal#3842 closes zama-ai/concrete-ml-internal#3869 closes zama-ai/concrete-ml-internal#3875 closes zama-ai/concrete-ml-internal#3855 closes zama-ai/concrete-ml-internal#3852

fd0r · 2023-09-21T09:27:34Z

Fixed @jfrery 's issue and squashed commits.

github-actions · 2023-09-21T10:12:04Z

Coverage passed ✅

Coverage details

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL    5954      0   100%

50 files skipped due to complete coverage.

jfrery

Looks good thanks

cla-bot bot added the cla-signed label Sep 13, 2023

fd0r force-pushed the llm_partial_fhe_pr branch 12 times, most recently from ac760f5 to 4504d5a Compare September 14, 2023 09:56

fd0r force-pushed the llm_partial_fhe_pr branch 4 times, most recently from c2a1289 to 0b6ef48 Compare September 18, 2023 17:21

fd0r marked this pull request as ready for review September 18, 2023 18:32

fd0r requested a review from a team as a code owner September 18, 2023 18:32

jfrery requested changes Sep 19, 2023

View reviewed changes

jfrery reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/torch/hybrid_model.py Show resolved Hide resolved

andrei-stoian-zama requested changes Sep 19, 2023

View reviewed changes

RomanBredehoft reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/deployment/fhe_client_server.py Show resolved Hide resolved

RomanBredehoft reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/onnx/convert.py Show resolved Hide resolved

RomanBredehoft reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/onnx/convert.py Outdated Show resolved Hide resolved

RomanBredehoft reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/quantization/post_training.py Outdated Show resolved Hide resolved

RomanBredehoft reviewed Sep 19, 2023

View reviewed changes

src/concrete/ml/torch/compile.py Outdated Show resolved Hide resolved

fd0r requested a review from andrei-stoian-zama September 20, 2023 11:21

fd0r force-pushed the llm_partial_fhe_pr branch 2 times, most recently from 0a414b8 to c964cf2 Compare September 20, 2023 12:38

jfrery reviewed Sep 20, 2023

View reviewed changes

use_case_examples/hybrid_model/compile_hybrid_llm.py Outdated Show resolved Hide resolved

fd0r force-pushed the llm_partial_fhe_pr branch from c964cf2 to ea204f4 Compare September 20, 2023 12:50

jfrery reviewed Sep 20, 2023

View reviewed changes

use_case_examples/hybrid_model/compile_hybrid_llm.py Outdated Show resolved Hide resolved

fd0r force-pushed the llm_partial_fhe_pr branch 2 times, most recently from 52be915 to 2c3cbb3 Compare September 20, 2023 14:35

fd0r requested a review from jfrery September 20, 2023 15:06

RomanBredehoft reviewed Sep 20, 2023

View reviewed changes

RomanBredehoft previously approved these changes Sep 20, 2023

View reviewed changes

jfrery previously approved these changes Sep 20, 2023

View reviewed changes

andrei-stoian-zama previously approved these changes Sep 21, 2023

View reviewed changes

jfrery reviewed Sep 21, 2023

View reviewed changes

use_case_examples/hybrid_model/requirements.txt Outdated

Copy link

Collaborator

jfrery Sep 21, 2023

Choose a reason for hiding this comment

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

It misses loguru I think?

jfrery reviewed Sep 21, 2023

View reviewed changes

use_case_examples/hybrid_model/serve_model.py Show resolved Hide resolved

jfrery reviewed Sep 21, 2023

View reviewed changes

use_case_examples/hybrid_model/infer_hybrid_llm_generate.py Show resolved Hide resolved

chore: implement hybrid model demo with GPT-2

6ab7d93

closes zama-ai/concrete-ml-internal#3842 closes zama-ai/concrete-ml-internal#3869 closes zama-ai/concrete-ml-internal#3875 closes zama-ai/concrete-ml-internal#3855 closes zama-ai/concrete-ml-internal#3852

fd0r dismissed stale reviews from andrei-stoian-zama, jfrery, and RomanBredehoft via 6ab7d93 September 21, 2023 09:03

fd0r force-pushed the llm_partial_fhe_pr branch from 2c3cbb3 to 6ab7d93 Compare September 21, 2023 09:03

fd0r requested review from jfrery, andrei-stoian-zama and RomanBredehoft September 21, 2023 11:02

jfrery approved these changes Sep 21, 2023

View reviewed changes

andrei-stoian-zama approved these changes Sep 21, 2023

View reviewed changes

fd0r merged commit f1d1490 into main Sep 21, 2023
8 checks passed

fd0r deleted the llm_partial_fhe_pr branch September 21, 2023 12:23

chore: implement hybrid model demo with GPT-2 #246

chore: implement hybrid model demo with GPT-2 #246

Conversation

fd0r commented Sep 13, 2023 • edited by andrei-stoian-zama Loading

andrei-stoian-zama commented Sep 15, 2023

fd0r commented Sep 15, 2023

jfrery left a 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

jfrery Sep 19, 2023 • edited Loading

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

andrei-stoian-zama left a 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

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

RomanBredehoft Sep 19, 2023 • edited Loading

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

RomanBredehoft left a comment

Choose a reason for hiding this comment

jfrery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fd0r commented Sep 21, 2023

github-actions bot commented Sep 21, 2023

Coverage passed ✅

jfrery left a comment

Choose a reason for hiding this comment

fd0r commented Sep 13, 2023 •

edited by andrei-stoian-zama

Loading

jfrery Sep 19, 2023 •

edited

Loading

RomanBredehoft Sep 19, 2023 •

edited

Loading