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

Disable signature related functions in scheduled fuzzer runs #9420

Closed

Conversation

assignUser
Copy link
Collaborator

@assignUser assignUser commented Apr 9, 2024

For the scheduled runs it doesn't make sense to run the signature related steps, specifically generating the signature cache might clobber (through a newer date, not by overwriting) newer stashes on main leading to false positives.

This PR also fixes issues with the generation of the baseline signature stash only happening in the scheduled jobs. The fuzzer is now also run on merge (against the parent commit on main) and will stash the signatures afterwards for use in PRs. The signatures stash key was amended with the sha so that we can ensure that the signatures match the merge base of the PR or generate them if they are (not) yet available.

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

netlify bot commented Apr 9, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2168a8e
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6617293585146b00089d85aa

@kgpai
Copy link
Contributor

kgpai commented Apr 9, 2024

@assignUser The signature change shouldnt fail right since you havent made any signature changes here ?

@kgpai kgpai self-requested a review April 9, 2024 20:44
@assignUser
Copy link
Collaborator Author

Ah I see what's happening. I forgot that this job doesn't actually trigger on merge (push) only in PRs and scheduled, so the signature stash is basically always outdated. Do we want to enable it on merge? or just update the signature stash (I can do that in on of the other jobs that runs anyway)

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, some questions though

.github/workflows/scheduled.yml Show resolved Hide resolved
.github/workflows/scheduled.yml Show resolved Hide resolved
@assignUser assignUser requested a review from kgpai April 10, 2024 17:11
@kgpai
Copy link
Contributor

kgpai commented Apr 10, 2024

@assignUser can you fix the conflict ? I will work on merging it.

@assignUser
Copy link
Collaborator Author

@kgpai I have rebased and properly fixed the save step, both in this workflow with continue-on-error and in the action itself by downgrading the actions/upload-artifacts version (it seems a recent update to an underlying npm package is causing this).

@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 cef85a1.

Copy link

Conbench analyzed the 1 benchmark run on commit cef85a19.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@zhli1142015
Copy link
Contributor

@assignUser , looks some CI jos are not triggered with latest main branch, https://github.com/facebookincubator/velox/actions/runs/8646131285, can you help check?
The workflow is not valid. .github/workflows/linux-build.yml (Line: 53, Col: 9): Unexpected symbol: '"facebookincubator/velox"'. Located at position 22 within expression: github.repository == "facebookincubator/velox" .github/workflows/linux-build.yml (Line: 118, Col: 9): Unexpected symbol: '"facebookincubator/velox"'. Located at position 22 within expression: github.repository == "facebookincubator/velox"

@assignUser
Copy link
Collaborator Author

@zhli1142015 Sorry for the inconvenience fix is here #9451

@zhli1142015
Copy link
Contributor

@zhli1142015 Sorry for the inconvenience fix is here #9451

Thanks

facebook-github-bot pushed a commit that referenced this pull request Apr 11, 2024
Summary:
Within `if:`/`${{}}`only single quotes are allowed, due to the fact that broken workflows don't show up in the check suite it got overlooked in #9420

Pull Request resolved: #9451

Reviewed By: bikramSingh91

Differential Revision: D56022463

Pulled By: kgpai

fbshipit-source-id: 79102cc2856db1e18fc4729ee55d602434861e0b
yanngyoung pushed a commit to yanngyoung/velox that referenced this pull request Apr 12, 2024
…kincubator#9420)

Summary:
For the scheduled runs it doesn't make sense to run the signature related steps, specifically generating the signature cache might clobber (through a newer date,  not by overwriting) newer stashes on main leading to false positives.

This PR also fixes issues with the generation of the baseline signature stash only happening in the scheduled jobs. The fuzzer is now  also run on merge (against the parent commit on main) and will stash the signatures afterwards for use in PRs. The signatures stash key was amended with the sha so that we can ensure that the signatures match the merge base of the PR or generate them if they are (not) yet available.

Pull Request resolved: facebookincubator#9420

Reviewed By: kagamiori, mbasmanova

Differential Revision: D55998367

Pulled By: kgpai

fbshipit-source-id: f4198f967de718c2ae1e0029091247c654297315
yanngyoung pushed a commit to yanngyoung/velox that referenced this pull request Apr 12, 2024
Summary:
Within `if:`/`${{}}`only single quotes are allowed, due to the fact that broken workflows don't show up in the check suite it got overlooked in facebookincubator#9420

Pull Request resolved: facebookincubator#9451

Reviewed By: bikramSingh91

Differential Revision: D56022463

Pulled By: kgpai

fbshipit-source-id: 79102cc2856db1e18fc4729ee55d602434861e0b
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
…kincubator#9420)

Summary:
For the scheduled runs it doesn't make sense to run the signature related steps, specifically generating the signature cache might clobber (through a newer date,  not by overwriting) newer stashes on main leading to false positives.

This PR also fixes issues with the generation of the baseline signature stash only happening in the scheduled jobs. The fuzzer is now  also run on merge (against the parent commit on main) and will stash the signatures afterwards for use in PRs. The signatures stash key was amended with the sha so that we can ensure that the signatures match the merge base of the PR or generate them if they are (not) yet available.

Pull Request resolved: facebookincubator#9420

Reviewed By: kagamiori, mbasmanova

Differential Revision: D55998367

Pulled By: kgpai

fbshipit-source-id: f4198f967de718c2ae1e0029091247c654297315
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
Within `if:`/`${{}}`only single quotes are allowed, due to the fact that broken workflows don't show up in the check suite it got overlooked in facebookincubator#9420

Pull Request resolved: facebookincubator#9451

Reviewed By: bikramSingh91

Differential Revision: D56022463

Pulled By: kgpai

fbshipit-source-id: 79102cc2856db1e18fc4729ee55d602434861e0b
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.

PR's failing with Signature check failure
4 participants