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

chore: integrate input compression #616

Merged
merged 12 commits into from
Apr 29, 2024

Conversation

RomanBredehoft
Copy link
Contributor

@RomanBredehoft RomanBredehoft commented Apr 16, 2024

enable input ciphertext compression by default through an environmental variable and add a test to make sure the size is indeed smaller

based on experiments, this feature can reduce input ciphertexts of hundreds of MB to less than 1 MB. Also, no significant slowdown have been observed for the inference time executions, expect with fully levelled circuits (where the inference can take a few hundreds milliseconds more).

For example :

closes https://github.com/zama-ai/concrete-ml-internal/issues/4365

@cla-bot cla-bot bot added the cla-signed label Apr 16, 2024
@RomanBredehoft RomanBredehoft changed the title chore: integrate input compression chore: integrate input compression [BLOCKED BY CP] Apr 16, 2024
@RomanBredehoft RomanBredehoft force-pushed the feat/integrate_input_compression_4365 branch from b0a2fc1 to 4bf970f Compare April 19, 2024 09:28
@RomanBredehoft RomanBredehoft changed the title chore: integrate input compression [BLOCKED BY CP] chore: integrate input compression Apr 19, 2024
@RomanBredehoft RomanBredehoft force-pushed the feat/integrate_input_compression_4365 branch 3 times, most recently from f8f2a51 to 2c68d26 Compare April 24, 2024 08:29
@RomanBredehoft RomanBredehoft marked this pull request as ready for review April 25, 2024 07:57
@RomanBredehoft RomanBredehoft requested a review from a team as a code owner April 25, 2024 07:57
@RomanBredehoft RomanBredehoft marked this pull request as draft April 25, 2024 07:59
@RomanBredehoft RomanBredehoft force-pushed the feat/integrate_input_compression_4365 branch from 740d5dd to f71dd46 Compare April 26, 2024 10:10
@RomanBredehoft RomanBredehoft marked this pull request as ready for review April 26, 2024 15:15
fd0r
fd0r previously approved these changes Apr 29, 2024
Copy link
Contributor

@fd0r fd0r left a comment

Choose a reason for hiding this comment

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

  • Well tested
  • Fixes an old ConcreteNumpy name
  • Properly integrates this new feature with an env var

Looks good to me, thanks for integrating this!

Looking at this I'm wondering if we shouldn't expose the compilation options to our users with the use of keyword arguments in the future. Also not sure why this is set through a keyword argument instead of an attribute in the Configuration object.

@RomanBredehoft
Copy link
Contributor Author

Looking at this I'm wondering if we shouldn't expose the compilation options to our users with the use of keyword arguments in the future.

we can talk about it at the weekly but I think we want to avoid having many non-ml parameters on CML side (cf #519 (comment))

Also not sure why this is set through a keyword argument instead of an attribute in the Configuration object.

in CP you can either pass kwargs or use a config object, if both are set than the value from the kwargs is taken, so I think it's best to do it like this since we want to make it default !

@fd0r
Copy link
Contributor

fd0r commented Apr 29, 2024

avoid having many non-ml parameters on CML side

Makes sense

if both are set than the value from the kwargs is taken

That's imo a confusing behavior for the users, we should probably talk about it with @umut-sahin. Would be great to have a Configuration object with CML defaults or something like that.

@umut-sahin
Copy link

I think it makes sense, kwargs are like a the last place you can set configuration, right within the compile call. You can have a base configuration and overwrite it with kwargs for example instead of changing the base configuration.

Copy link
Collaborator

@andrei-stoian-zama andrei-stoian-zama left a comment

Choose a reason for hiding this comment

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

Good for me, thanks!

Copy link

Coverage passed ✅

Coverage details

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

59 files skipped due to complete coverage.

@RomanBredehoft RomanBredehoft merged commit 4e7ef50 into main Apr 29, 2024
11 checks passed
@RomanBredehoft RomanBredehoft deleted the feat/integrate_input_compression_4365 branch April 29, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants