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

Move signature check and bias fuzzer to gha #9191

Closed
wants to merge 51 commits into from

Conversation

assignUser
Copy link
Collaborator

@assignUser assignUser commented Mar 21, 2024

Move the signature check and bias fuzzer from CCI to GHA.

@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 21, 2024
Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for meta-velox canceled.

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

@assignUser assignUser force-pushed the bias-fuzzer-gha branch 2 times, most recently from c39e840 to 789e697 Compare March 23, 2024 02:57
@assignUser assignUser changed the title [WIP] Add bias fuzzer to gha Add bias fuzzer to gha Mar 25, 2024
@assignUser assignUser marked this pull request as ready for review March 25, 2024 15:23
@assignUser assignUser requested a review from kgpai March 25, 2024 15:24
@assignUser assignUser changed the title Add bias fuzzer to gha Move signature check and bias fuzzer to gha Mar 26, 2024
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
.github/workflows/scheduled.yml Show resolved Hide resolved
.github/workflows/scheduled.yml Outdated Show resolved Hide resolved
.github/workflows/scheduled.yml Show resolved Hide resolved
.github/workflows/scheduled.yml Outdated Show resolved Hide resolved
.github/workflows/scheduled.yml Show resolved Hide resolved
.github/workflows/scheduled.yml Outdated Show resolved Hide resolved
@assignUser assignUser requested a review from kgpai March 27, 2024 18:09
@assignUser assignUser force-pushed the bias-fuzzer-gha branch 2 times, most recently from 9b79e81 to 07e1e87 Compare March 29, 2024 03:14
@assignUser
Copy link
Collaborator Author

assignUser commented Mar 29, 2024

@kgpai the bias fuzzer/signature check stuff works now.

@kgpai
Copy link
Contributor

kgpai commented Mar 29, 2024

Thanks for the changes @assignUser

@assignUser
Copy link
Collaborator Author

I reverted the test changes, this should be ready to go now! I also removed the remaining cci bits. I will cleanup scripts/ in a follow up.

scripts/signature.py Show resolved Hide resolved
scripts/adapters.dockerfile Show resolved Hide resolved
scripts/signature.py Outdated Show resolved Hide resolved
@@ -527,3 +635,21 @@ jobs:
path: |
/tmp/aggregate_fuzzer_repro
/tmp/server.log

surface-signature-errors:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be separate workflow, cant we do it at the end of the failing workflow ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is meant to surface the issue quickly (e.g. in case it wasn't intentional), if we add it to the bias fuzzer it will show up an hour later. The job takes a few seconds so it's not like it costs anything ^^

./velox_expression_fuzzer_test \
--seed ${RANDOM} \
--lazy_vector_generation_ratio 0.2 \
--assign_function_tickets $(cat /tmp/signatures/presto_bias_functions) \
Copy link
Contributor

Choose a reason for hiding this comment

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

what if presto_bias_functions is empty - shouldnt there be a check above ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An empty file will not be created due to this. And thus this step won't run. Previously an empty file might have been created due to the way argsparse.FileType works (it opens the file when the args are parsed) but I removed that.

.github/workflows/scheduled.yml Show resolved Hide resolved
@kgpai
Copy link
Contributor

kgpai commented Apr 1, 2024

Can you make the circleci configuration empty , or a no/op ? This is because currently it shows an error if there is no configuration. I will request a deactivation of cci and then I can remove it.

Copy link
Contributor

@kgpai kgpai 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, If you can make an no-op empty dist-compile for CCI, then I can deactivate it and then we can subsequently remove it.

@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 8acb197.

Copy link

Conbench analyzed the 1 benchmark run on commit 8acb197e.

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
Summary:
Move the signature check and bias fuzzer from CCI to GHA.

Pull Request resolved: facebookincubator#9191

Reviewed By: Yuhta

Differential Revision: D55599384

Pulled By: kgpai

fbshipit-source-id: b998a064319f451ebe2d3508f3dbca3d165a669a
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
Move the signature check and bias fuzzer from CCI to GHA.

Pull Request resolved: facebookincubator#9191

Reviewed By: Yuhta

Differential Revision: D55599384

Pulled By: kgpai

fbshipit-source-id: b998a064319f451ebe2d3508f3dbca3d165a669a
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