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

Add support for Aggregate signatures in PyVelox and Signatures.py #9104

Closed
wants to merge 1 commit into from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Mar 16, 2024

What
This PR adds support for aggregate signatures in PyVelox and export these signatures using signature.py

Why
Support for aggregate signatures was missing previously in our biasing logic to make sure that changes in aggregate functions are backwards compatible. This first PR adds support for these aggregate signatures so that we can use them to track changes in aggregate signatures across commits. A subsequent PR will add support for biasing and erroring if we detect compatible/incompatible changes with newly added or modified aggregates.

@kgpai kgpai requested a review from assignUser March 16, 2024 00:20
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 16, 2024
@kgpai kgpai requested a review from kagamiori March 16, 2024 00:20
Copy link

netlify bot commented Mar 16, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 8e2a32e
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/660c71b416f7110008fea45c

@kgpai kgpai force-pushed the bias_aggregation_fuzzer branch from 94c2977 to 488c980 Compare March 16, 2024 00:21
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

LGTM. Please also let @assignUser take a look.

Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

LGTM I can rebase #9191 on this and prepare the actual sig checking too

@kgpai kgpai force-pushed the bias_aggregation_fuzzer branch from 488c980 to a8e160a Compare April 1, 2024 20:40
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kgpai kgpai force-pushed the bias_aggregation_fuzzer branch from a8e160a to 3a461c0 Compare April 1, 2024 22:07
@kgpai kgpai force-pushed the bias_aggregation_fuzzer branch from 3a461c0 to 8e2a32e Compare April 2, 2024 20:59
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in ece3961.

Copy link

Conbench analyzed the 1 benchmark run on commit ece3961b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Apr 4, 2024
…cebookincubator#9104)

Summary:
**What**
This PR adds support for aggregate signatures in PyVelox and export these signatures using signature.py

**Why**
Support for aggregate signatures was missing previously in our biasing logic to make sure that changes in aggregate functions are backwards compatible. This first PR adds support for these aggregate signatures so that we can use them to track changes in aggregate signatures across commits. A subsequent PR will add support for biasing and erroring if we detect compatible/incompatible changes with newly added or modified aggregates.

Pull Request resolved: facebookincubator#9104

Reviewed By: kagamiori

Differential Revision: D54977056

Pulled By: kgpai

fbshipit-source-id: aff940ff3b7b7eca2b825f67d3c6de2b10a0baad
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…cebookincubator#9104)

Summary:
**What**
This PR adds support for aggregate signatures in PyVelox and export these signatures using signature.py

**Why**
Support for aggregate signatures was missing previously in our biasing logic to make sure that changes in aggregate functions are backwards compatible. This first PR adds support for these aggregate signatures so that we can use them to track changes in aggregate signatures across commits. A subsequent PR will add support for biasing and erroring if we detect compatible/incompatible changes with newly added or modified aggregates.

Pull Request resolved: facebookincubator#9104

Reviewed By: kagamiori

Differential Revision: D54977056

Pulled By: kgpai

fbshipit-source-id: aff940ff3b7b7eca2b825f67d3c6de2b10a0baad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants