-
Notifications
You must be signed in to change notification settings - Fork 130
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
[tfhers compatibility] frontend support #820
Conversation
16a7aa0
to
4db9e8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong start 💪
tfhers_to_native = { | ||
inputs_and_output_share_precision, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about from native?
Also is this the condition we really want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We basically want to have the input precision (the one of the tfhers integer) to rule it, as tfhers inputs should come from an external computation. We can't just decrease tfhers int bitwidth, so the native one have to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we assign it the precision obtained from the inputset? (e.g., maybe all values that will be given to the circuit is smaller than 10 so it's enough to process 5 chunks from TFHE-rs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really depends on the usecase, but as I'm currently seeing it, we don't control the tfhers computation part, and we aren't asked to look at it, and what are its outputs, all we know is it's output bitwidth. So we shouldn't have a meaningful inputset to use here to try to know how many chunks should we use.
Cc @BourgerieQuentin wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that'll still be how the circuit is compiled. If they want to have the full range, they should give it in the inputset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it was what we said, so the output precision can differ
4db9e8f
to
668f4e8
Compare
@@ -116,6 +117,20 @@ def typeof(self, value: Union[ValueDescription, Node]) -> ConversionType: | |||
assert isinstance(value.dtype, Integer) | |||
bit_width = value.dtype.bit_width | |||
|
|||
# TODO: what about the element type? only unsigned? or not eint at all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it depends on how it's done in TFHE-rs. We should think about which operations would be performed on this type and use eint or esint accordingly. We should also test correct transfer of sign (if input was -3 in TFHE-rs, it should correctly be converted to -3 in Concrete).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will have at least to see how they do sign encoding and consider that as part of the MLIR conversion
tfhers_to_native = { | ||
inputs_and_output_share_precision, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we assign it the precision obtained from the inputset? (e.g., maybe all values that will be given to the circuit is smaller than 10 so it's enough to process 5 chunks from TFHE-rs)
int8 = partial(TFHERSIntegerType, True, 8) | ||
uint8 = partial(TFHERSIntegerType, False, 8) | ||
int16 = partial(TFHERSIntegerType, True, 16) | ||
uint16 = partial(TFHERSIntegerType, False, 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an _ in the beginning would be enough IMO 🙂
tfhers_to_native = { | ||
inputs_and_output_share_precision, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it was what we said, so the output precision can differ
@slab-ci concrete-python-tests-linux |
e812d77
to
0470ba3
Compare
this comes from the need of supporting tfhers integers, which would get erased by this recreation of the dtype, so instead we would update the dtype inplace
0470ba3
to
41389f8
Compare
- add new tfhers types - add conversion functions between concrete and tfhers types - support conversion function in the compilation pipeline
41389f8
to
94b3e52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small remark, looks good!
from concrete import fhe | ||
from concrete.fhe import tfhers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better if we can do:
from concrete import fhe, tfhers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means a different module, and I'm not sure it makes sense as they would depend on each other
No description provided.