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
ARROW-17751: [Go][Benchmarking] Add Go Benchmark Script #14148
Conversation
|
This looks good to me — the CI failures don't look related, are they? |
@jonkeane you are correct, the CI failure is not related to this change at all. |
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.
Looking at the top of .github/workflows/go.yml
(which it won't let me comment on directly), it looks like this will run on more places than the default branch:
on:
push:
paths:
- '.github/workflows/go.yml'
- 'ci/docker/*_go.dockerfile'
- 'ci/scripts/go_*'
- 'go/**'
pull_request:
paths:
- '.github/workflows/go.yml'
- 'ci/docker/*_go.dockerfile'
- 'ci/docker/**'
- 'ci/scripts/go_*'
- 'go/**'
...so run_reason
needs to be flipped from actions (state passed through as an arg or env var, probably). It should be "commit"
when on master
, "pull request"
when on a PR, and if this runs elsewhere on a branch...maybe "branch"
; we haven't done that previously.
Once that's resolved, this looks good to go!
.github/workflows/go.yml
Outdated
@@ -183,13 +192,13 @@ jobs: | |||
run: ci/scripts/go_build.sh $(pwd) | |||
- name: Test | |||
shell: bash | |||
run: ci/scripts/go_test.sh $(pwd) | |||
run: ci/scripts/go_test.sh $(pwd) |
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.
run: ci/scripts/go_test.sh $(pwd) | |
run: ci/scripts/go_test.sh $(pwd) |
Maybe try saving with "strip trailing whitespace"? Just trying to prevent future whitespace diffs
ci/scripts/go_bench_adapt.py
Outdated
"name": pieces[0], | ||
"params": '/'.join(pieces[1:]), | ||
}, | ||
run_reason=f'commit', |
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 this only going to run on master? If so, this is fine; if not, it needs to be flipped from gh actions
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.
Updated to have the reason derived by checking the branch ref_name
for master
-> 'commit' otherwise it uses branch
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.
The workflow yaml looks good, tested on your fork?
.github/workflows/go.yml
Outdated
@@ -183,13 +192,13 @@ jobs: | |||
run: ci/scripts/go_build.sh $(pwd) | |||
- name: Test | |||
shell: bash | |||
run: ci/scripts/go_test.sh $(pwd) | |||
run: ci/scripts/go_test.sh $(pwd) |
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.
nit
run: ci/scripts/go_test.sh $(pwd) | |
run: ci/scripts/go_test.sh $(pwd) |
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.
ran trim trailing whitespace to fix that :)
And yea, tested it on my fork before I removed the runs on PRs to have it only run on pushes to apache/arrow repo.
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.
lgtm!
No description provided.