-
Notifications
You must be signed in to change notification settings - Fork 145
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
docs(frontend): adding a use-case for private information retrieval #1040
Conversation
9995f7a
to
8f259df
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.
Could you create a .py
script as well. It'll be necessary for testing and benchmarks. Maybe even do those directly, wdyt?
Very cool example btw!
Let me have a look to make a .py yes. I am currently installing the new computer and I have an issue running the nb: it crashes when doing the encrypted_y = circuit.run(encrypted_x, database) I am investigating. |
OK my issue is some:
We have already seen that kind of things but I thought it was just for mac intel. It's not. |
Could you share it in |
I think I fixed that with zama-ai/concrete-ml#870 |
058e9a2
to
8090b10
Compare
33fee51
to
063db93
Compare
@umut-sahin is it better now? I have adressed your comments I think + added some tests in pytest. I haven't done the benchmark part, I am less sure how to do it but maybe we can do it together? |
063db93
to
81964d8
Compare
Tests seem to fail, could you fix it before I give another review? |
sure my bad, sorry for that, I think they worked locally but let me see |
81964d8
to
2b3533b
Compare
|
CC @aquint-zama and @BourgerieQuentin if you want to have a look and review a bit the notebook |
You'll have the ability to reset pretty soon 😉 |
fb6cbe0
to
0190e6b
Compare
@umut-sahin the tests look good! |
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 there shouldn't be a pir_utils.py
, there should be pir.py
, which should be runable on it's own (e.g., to print an example, or assert some things, show timings).
Within it, there should be a class named PrivateInfomationRetrival
, it should accept configuration: Optional[fhe.Configuration] = None
and compiled: bool = True
for its constructor.
Lastly, the test logic should be defined in tests
not in the example itself.
Sorry it's not a small request, but it's for being consistent across the examples!
Let's discuss it, then. What I want is:
That's why I did what I did. |
I'm just saying we don't have
|
I like it for the first three parts, but then I really think it's at the price of having nice notebook-tutorials. Those notebook-tutorials are really easier for the users, especially the beginners, to follow and appreciate. |
We're not in the same view, let's see with another person to break the tie. @BourgerieQuentin , what do you think please? I am fine refactoring all of this, but I don't think it goes for the best of the final users. |
I am starting a |
@BourgerieQuentin , we debated a bit with @umut-sahin and came to the following proposal: To test the CP examples, we would either do it:
What do you think of this proposal? An alternative approach was:
but we thought it would be a bit over engineering to do that. |
Ok fine for me 👍 |
Awesome so I'm going to remove my changes in *.py, and just keep the notebook, which will be tested with the other PR. |
4840db4
to
9e2c021
Compare
So now, it's just the notebook to review please. |
9e2c021
to
f513290
Compare
Pushed changes requested by @BourgerieQuentin , let me see if the build is still green |
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 added a very minor comment, so good to me for merging. Thanks for this cool example 👍
38aa16d
to
6d79602
Compare
Thanks a lot @umut-sahin / @BourgerieQuentin , let's merge! |
6d79602
to
ea91dbf
Compare
ea91dbf
to
95c5f13
Compare
Work in progress, I've made experiments for PIR with Concrete. I think we can find use-cases for which it's not that bad.