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): add support for wires to concrete-python #845

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

aPere3
Copy link
Collaborator

@aPere3 aPere3 commented May 23, 2024

@cla-bot cla-bot bot added the cla-signed label May 23, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Benchmark

Benchmark suite Current: 494a5fc Previous: 66d70b7 Ratio
v0 PBS table generation 60218721 ns/iter (± 302809) 59802486 ns/iter (± 1213879) 1.01
v0 PBS simulate dag table generation 38003941 ns/iter (± 284418) 37033984 ns/iter (± 287177) 1.03
v0 WoP-PBS table generation 68112466 ns/iter (± 511478) 67839950 ns/iter (± 436131) 1.00

This comment was automatically generated by workflow using github-action-benchmark.

@aPere3 aPere3 force-pushed the alex/concrete_python_wires branch 2 times, most recently from 8f548ef to ad7fe6b Compare May 28, 2024 07:16
@aPere3 aPere3 marked this pull request as ready for review May 28, 2024 07:17
@aPere3 aPere3 force-pushed the alex/concrete_python_wires branch 9 times, most recently from c7edfe0 to 1fd6ef2 Compare May 30, 2024 07:09
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.

Looks good to me, I'm just think that the default composition of modules should be fully composable as the previous behavior. wdyt?

@umut-sahin
Copy link
Contributor

the default composition of modules should be fully composable as the previous behavior

I don't think we should do this by default. There is a reason why we introduced wires, it's more optimal, and it should be the default choice.

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.

Python side looks good to me, only a few comments. Though I'd love to see the initial wire API we designed as well, it looks much better to me!

@BourgerieQuentin
Copy link
Member

the default composition of modules should be fully composable as the previous behavior

I don't think we should do this by default. There is a reason why we introduced wires, it's more optimal, and it should be the default choice.

For me FullyComposable should be the default one, as it make things works then you optimize it using wiring. Then it is the previous behavior so we don't break the legacy.

@aPere3
Copy link
Collaborator Author

aPere3 commented Jun 6, 2024

For me FullyComposable should be the default one, as it make things works then you optimize it using wiring. Then it is the previous behavior so we don't break the legacy.

Agreed. I changed it this way.

@aPere3 aPere3 force-pushed the alex/concrete_python_wires branch 6 times, most recently from d04ac7c to 6bf2d12 Compare June 10, 2024 13:47
@aPere3 aPere3 force-pushed the alex/concrete_python_wires branch from 63079da to 494a5fc Compare June 10, 2024 15:56
@aPere3 aPere3 merged commit 5185940 into main Jun 11, 2024
37 of 40 checks passed
@aPere3 aPere3 deleted the alex/concrete_python_wires branch June 11, 2024 11:23
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.

3 participants