-
Notifications
You must be signed in to change notification settings - Fork 147
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
[Optimizer] support external partitions #915
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0cfc9e3
feat(optimizer): add change_partition operator
youben11 8d0ba3d
feat(optimizer): partition with external partitions
youben11 46571d9
feat(optimizer): block macro parameters in external partitions
youben11 eb9ea06
feat(optimizer): constrain optimizer with external max_variance
youben11 8b36b06
refactor(optimizer): move external partition during change_part creation
youben11 b81e13d
test(optimizer): mix tfhers and low_precision partitions
youben11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it really possible to have
None, None
? OrSome(a), Some(a)
?If so, maybe it would be worth having an assert when adding the op !
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 agree that (None, None), doesn't make sense, but having them both is something we decided not to restrict for now, but we will see. I will add an assert for the first case.
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.
Hmm actually it is not clear for me how this is supposed to work. Only one of them is expected to be something (though you support having both set) ? And the other (none) is supposed to signal the fact that it is free to have any partition ?
My original point was checking that both were something different. Maybe we could have a quick chat, so that I am sure I got everything right ?
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 the "change partition" is just an informative flag of the origin/destination partition for circuit inputs and results. We choosed to keep it more generic and compatible with eventual future use in the middle of a circuit.
Current used form are
ChangePartition(Some, None) on inputs
ChangePartition(None, Some) on results
E.g. future use
ChangePartition(None, None)
could hint that if a conflict exists the partition change should happen here and not somewhere else.ChangePartition(None, Some), could be use to express manually where a transition occurs, which is forced automatically today by the optimizer policy but better manual decision could be taken.
Also it's translated from MLIR, so it could be used there to have multi partition annoted code instead of relying on optimizer choosing partition and parameters.