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

feat(frontend): wire tracing #946

Merged
merged 1 commit into from
Sep 2, 2024
Merged

feat(frontend): wire tracing #946

merged 1 commit into from
Sep 2, 2024

Conversation

aPere3
Copy link
Collaborator

@aPere3 aPere3 commented Jul 18, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 18, 2024
Copy link
Contributor

@bcm-at-zama bcm-at-zama left a comment

Choose a reason for hiding this comment

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

Maybe you could update the doc as well, in the same PR? Doing so, it would help me understand how the new feature works. Thanks!

@bcm-at-zama bcm-at-zama requested a review from umut-sahin July 22, 2024 08:20
Copy link
Contributor

@bcm-at-zama bcm-at-zama left a comment

Choose a reason for hiding this comment

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

Some more comments.

In general:

  • please do not have me as a single reviewer. I added @umut-sahin
  • I would say we miss a lot of comments
  • I would prefer to have the doc here as well, to understand how it works, and be able to say what I think
  • ideally, the tests would be with a more-complex-control-flow function. Else, I am afraid we don't really check the auto wiring.

@aPere3 aPere3 marked this pull request as draft July 22, 2024 09:26
Copy link
Contributor

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

The API is usable, but I think we talked about some kind of pipeline API, in which the wires would be detected + a single inputset would be given for the whole pipeline. Did we give up on that idea, or is this one just a temporary solution until then?

@BourgerieQuentin
Copy link
Member

I schedule a meeting when @aPere3, with @umut-sahin come back to discuss of that and the API.

@aPere3 aPere3 force-pushed the alex/wire_tracing branch 2 times, most recently from c1fd921 to 3915d54 Compare August 23, 2024 14:22
@aPere3 aPere3 marked this pull request as ready for review August 23, 2024 14:23
@aPere3 aPere3 force-pushed the alex/wire_tracing branch from 3915d54 to d23c74b Compare August 23, 2024 14:26
@aPere3 aPere3 force-pushed the alex/wire_tracing branch 3 times, most recently from 2332bfd to fa935bd Compare August 27, 2024 12:10
Copy link
Member

@BourgerieQuentin BourgerieQuentin left a comment

Choose a reason for hiding this comment

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

Ok look good to me, I'm just wondering in term of API if we could declare the pipeline as a composition policy directly?
Something like

@fhe.module()
class Module:
   ...

   composition = fhe.Pipeline(inputset, lambda sample: Module.c(Module.b(Module.a(s))))

@aPere3
Copy link
Collaborator Author

aPere3 commented Aug 29, 2024

Ok look good to me, I'm just wondering in term of API if we could declare the pipeline as a composition policy directly? Something like

@fhe.module()
class Module:
   ...

   composition = fhe.Pipeline(inputset, lambda sample: Module.c(Module.b(Module.a(s))))

Module would not be defined here, so we would need to pass the Module to the lambda. We talked about that with @umut-sahin , and settled on the current API.

@aPere3 aPere3 force-pushed the alex/wire_tracing branch 2 times, most recently from 89170d1 to 78ec553 Compare August 29, 2024 09:35
@BourgerieQuentin
Copy link
Member

Ok look good to me, I'm just wondering in term of API if we could declare the pipeline as a composition policy directly? Something like

@fhe.module()
class Module:
   ...

   composition = fhe.Pipeline(inputset, lambda sample: Module.c(Module.b(Module.a(s))))

Module would not be defined here, so we would need to pass the Module to the lambda. We talked about that with @umut-sahin , and settled on the current API.

Ok that's what I expected but as I'm not an python expert and as we can do weird things in python I'm wondering if it was possible :). 👍

So I think it could be great to not to have to set an empty fhe.Wired composition rule just for readability. But that can be done in a next PR. wdyt?

@BourgerieQuentin
Copy link
Member

@yuxizama the PR is ready for doc review

@aPere3
Copy link
Collaborator Author

aPere3 commented Aug 29, 2024

So I think it could be great to not to have to set an empty fhe.Wired composition rule just for readability. But that can be done in a next PR. wdyt?

Yes, I was not sure about the what you guys would prefer so I left it this way, but I'll change it to automatically set the proper policy.

No worries, I'll do it there. It's a small change, and CI is still not green anyway.

@aPere3 aPere3 force-pushed the alex/wire_tracing branch from 78ec553 to 2875122 Compare August 29, 2024 12:31
Copy link
Contributor

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

It looks good to me, I'm just curious about how it'll work in practice.

@aPere3
Copy link
Collaborator Author

aPere3 commented Aug 29, 2024

It looks good to me, I'm just curious about how it'll work in practice.

What do you mean ?

Copy link
Contributor

@bcm-at-zama bcm-at-zama left a comment

Choose a reason for hiding this comment

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

Thanks for the doc @aPere3 . Just asking a change for SHA1, for also use modules please, thanks

Copy link
Contributor

@yuxizama yuxizama left a comment

Choose a reason for hiding this comment

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

Some suggestions, thank you!

@aPere3 aPere3 force-pushed the alex/wire_tracing branch 2 times, most recently from 8993981 to 3cb9ecd Compare August 30, 2024 09:21
@aPere3 aPere3 requested a review from bcm-at-zama August 30, 2024 09:21
Copy link
Contributor

@bcm-at-zama bcm-at-zama left a comment

Choose a reason for hiding this comment

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

Thanks @aPere3

@aPere3 aPere3 merged commit e675a75 into main Sep 2, 2024
29 of 33 checks passed
@aPere3 aPere3 deleted the alex/wire_tracing branch September 2, 2024 08:50
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.

5 participants