You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/02/13 16:52:20 UTC
[GitHub] [arrow] zeroshade opened a new pull request, #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
zeroshade opened a new pull request, #34167:
URL: https://github.com/apache/arrow/pull/34167
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
A bug was found in the `noasm` build for the Go lib in #34044 so adding a CI run with the `noasm` tag should prevent that from happening again in the future.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
Updating the `go_test.sh` script to allow for an env var to tell it to add the `noasm` tag to the `go test` command
Updating go.yml to add a `noasm` CI run
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade merged pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #34167:
URL: https://github.com/apache/arrow/pull/34167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34167:
URL: https://github.com/apache/arrow/pull/34167#issuecomment-1430482980
Benchmark runs are scheduled for baseline = a759ffc881c07a07c62170ab06c44a30067e1f14 and contender = 266f16668050d2c901328c11a6da2b31b01ea302. 266f16668050d2c901328c11a6da2b31b01ea302 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/5defc9f0425b4f0aa8f6864b045c08d3...b6414f4e0a5a4aeda59b1039b7430b40/)
[Failed :arrow_down:0.06% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/9c04ff1cc7174f45a3d73d2dcffab23a...4e4082058e0b4b0187025c7cc0f51b31/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/6db357ccc8d0437ea485411d47aab609...93a6d571c73c44edbaaac90eed6e8ced/)
[Finished :arrow_down:0.16% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/1dfee6d4b4e54a48aeb1fc1661d3c7b9...fd15f2f4fb9e43cfa4f3a2fd9888872b/)
Buildkite builds:
[Finished] [`266f1666` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2371)
[Failed] [`266f1666` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2401)
[Finished] [`266f1666` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2369)
[Finished] [`266f1666` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2393)
[Finished] [`a759ffc8` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2370)
[Failed] [`a759ffc8` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2400)
[Finished] [`a759ffc8` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2368)
[Finished] [`a759ffc8` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2392)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34167:
URL: https://github.com/apache/arrow/pull/34167#discussion_r1104860831
##########
.github/workflows/go.yml:
##########
@@ -369,3 +369,40 @@ jobs:
- name: Test
shell: bash
run: ci/scripts/go_test.sh $(pwd)
+
+ noasm:
+ name: AMD64 Debian 11 Go ${{ matrix.go }} - NOASM
+ runs-on: ubuntu-latest
+ if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+ timeout-minutes: 60
+ strategy:
+ fail-fast: false
+ matrix:
+ go: [1.17, 1.18]
+ include:
+ - go: 1.17
+ staticcheck: v0.2.2
+ - go: 1.18
+ staticcheck: v0.3.3
+ env:
+ GO: ${{ matrix.go }}
+ STATICCHECK: ${{ matrix.staticcheck }}
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Install go
+ uses: actions/setup-go@v3
+ with:
+ go-version: ${{ matrix.go }}
+ cache: true
+ cache-dependency-path: go/go.sum
+ - name: Install staticcheck
Review Comment:
side note, why isn't this one of the things archery runs? does it need to build the code to work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34167:
URL: https://github.com/apache/arrow/pull/34167#discussion_r1105183460
##########
.github/workflows/go.yml:
##########
@@ -369,3 +369,40 @@ jobs:
- name: Test
shell: bash
run: ci/scripts/go_test.sh $(pwd)
+
+ noasm:
+ name: AMD64 Debian 11 Go ${{ matrix.go }} - NOASM
+ runs-on: ubuntu-latest
+ if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+ timeout-minutes: 60
+ strategy:
+ fail-fast: false
+ matrix:
+ go: [1.17, 1.18]
+ include:
+ - go: 1.17
+ staticcheck: v0.2.2
+ - go: 1.18
+ staticcheck: v0.3.3
+ env:
+ GO: ${{ matrix.go }}
+ STATICCHECK: ${{ matrix.staticcheck }}
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Install go
+ uses: actions/setup-go@v3
+ with:
+ go-version: ${{ matrix.go }}
+ cache: true
+ cache-dependency-path: go/go.sum
+ - name: Install staticcheck
Review Comment:
@lidavidm we call staticcheck in the `go_test.sh` file so that's why I did it here, though technically we don't necessarily need it in all the pipelines but we should have it run in the both `asm` and `noasm` pipelines since it changes which files actually get included.
that said, I can definitely shoehorn this into one of the existing jobs instead as @assignUser requested :smile: I'll update this tomorrow to avoid making another job
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34167:
URL: https://github.com/apache/arrow/pull/34167#discussion_r1104932841
##########
.github/workflows/go.yml:
##########
@@ -369,3 +369,40 @@ jobs:
- name: Test
shell: bash
run: ci/scripts/go_test.sh $(pwd)
+
+ noasm:
+ name: AMD64 Debian 11 Go ${{ matrix.go }} - NOASM
+ runs-on: ubuntu-latest
+ if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+ timeout-minutes: 60
+ strategy:
+ fail-fast: false
+ matrix:
+ go: [1.17, 1.18]
+ include:
+ - go: 1.17
+ staticcheck: v0.2.2
+ - go: 1.18
+ staticcheck: v0.3.3
+ env:
+ GO: ${{ matrix.go }}
+ STATICCHECK: ${{ matrix.staticcheck }}
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Install go
+ uses: actions/setup-go@v3
+ with:
+ go-version: ${{ matrix.go }}
+ cache: true
+ cache-dependency-path: go/go.sum
+ - name: Install staticcheck
Review Comment:
It's part of the building of the Go docker images, which is why this step doesn't exist in jobs that use the archery images. I just didn't feel like making an entirely separate docker image for building/running with the `noasm` tag.
If you look at the primary jobs which use archery, they don't have a separate `install staticcheck` step, it's in the dockerfile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34167:
URL: https://github.com/apache/arrow/pull/34167#discussion_r1105105993
##########
.github/workflows/go.yml:
##########
@@ -369,3 +369,40 @@ jobs:
- name: Test
shell: bash
run: ci/scripts/go_test.sh $(pwd)
+
+ noasm:
+ name: AMD64 Debian 11 Go ${{ matrix.go }} - NOASM
+ runs-on: ubuntu-latest
+ if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+ timeout-minutes: 60
+ strategy:
+ fail-fast: false
+ matrix:
+ go: [1.17, 1.18]
+ include:
+ - go: 1.17
+ staticcheck: v0.2.2
+ - go: 1.18
+ staticcheck: v0.3.3
+ env:
+ GO: ${{ matrix.go }}
+ STATICCHECK: ${{ matrix.staticcheck }}
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Install go
+ uses: actions/setup-go@v3
+ with:
+ go-version: ${{ matrix.go }}
+ cache: true
+ cache-dependency-path: go/go.sum
+ - name: Install staticcheck
Review Comment:
Sure, I just mean that - is it necessary to run staticcheck separately in all pipelines, vs just once in the Archery pipeline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34167:
URL: https://github.com/apache/arrow/pull/34167#issuecomment-1430044621
@assignUser updated to just have `go_test.sh` run the tests twice, once without and once with the `noasm` tag. So all the existing jobs just get the additional run of tests using `noasm` now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34167:
URL: https://github.com/apache/arrow/pull/34167#issuecomment-1428281616
* Closes: #34055
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] assignUser commented on a diff in pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #34167:
URL: https://github.com/apache/arrow/pull/34167#discussion_r1105113975
##########
.github/workflows/go.yml:
##########
@@ -369,3 +369,40 @@ jobs:
- name: Test
shell: bash
run: ci/scripts/go_test.sh $(pwd)
+
+ noasm:
+ name: AMD64 Debian 11 Go ${{ matrix.go }} - NOASM
+ runs-on: ubuntu-latest
+ if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
+ timeout-minutes: 60
+ strategy:
+ fail-fast: false
+ matrix:
+ go: [1.17, 1.18]
+ include:
+ - go: 1.17
+ staticcheck: v0.2.2
+ - go: 1.18
+ staticcheck: v0.3.3
+ env:
+ GO: ${{ matrix.go }}
+ STATICCHECK: ${{ matrix.staticcheck }}
+ steps:
+ - name: Checkout Arrow
+ uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
+ submodules: recursive
+ - name: Install go
+ uses: actions/setup-go@v3
+ with:
+ go-version: ${{ matrix.go }}
+ cache: true
+ cache-dependency-path: go/go.sum
+ - name: Install staticcheck
Review Comment:
Can we not run `go_test.sh` with the noasm flag in one of the exisisting jobs instead of creating another 2 jobs? The go matrix is pretty big already :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34167: GH-34055: [Go][CI] Add test run in CI that uses noasm tag
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34167:
URL: https://github.com/apache/arrow/pull/34167#issuecomment-1430489199
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/9c04ff1cc7174f45a3d73d2dcffab23a...4e4082058e0b4b0187025c7cc0f51b31/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org