Simplify archery integration and disable JS temporarily #2449
Conversation
tustvold
force-pushed
the
simplify-integration-test-harness
branch
from
e3654b4
to
d03dd5b
Compare
.github/workflows/integration.yml
Outdated
| - name: Build Rust | ||
| run: ci/scripts/rust_build.sh . /build | ||
| - name: Build C++ | ||
| run: ci/scripts/cpp_build.sh . /build | ||
| - name: Build C# | ||
| run: ci/scripts/csharp_build.sh . /build | ||
| - name: Build Go | ||
| run: ci/scripts/go_build.sh . | ||
| - name: Build Java | ||
| run: ci/scripts/java_build.sh . /build | ||
| - name: Build JS | ||
| run: ci/scripts/js_build.sh . /build | ||
| - name: Install archery | ||
| run: pip install -e dev/archery |
There was a problem hiding this comment.
| run: ci/scripts/js_build.sh . /build | ||
| - name: Install archery | ||
| run: pip install -e dev/archery | ||
| - name: Run integration tests |
There was a problem hiding this comment.
tustvold
force-pushed
the
simplify-integration-test-harness
branch
5 times, most recently
from
496926e
to
37db501
Compare
tustvold
force-pushed
the
simplify-integration-test-harness
branch
17 times, most recently
from
39d2263
to
1dfa9a1
Compare
tustvold
force-pushed
the
simplify-integration-test-harness
branch
from
1dfa9a1
to
f80c3e0
Compare
tustvold
changed the title
|
See #2453 for an example of the failing JS build, which this disables |
There was a problem hiding this comment.
I am +0 on this change (I don't oppose it but I am not a fan either). I would like some other opinions
My hesitation with moving away from archery.py (which is complicated, as you note in the description) is that archery is used for testing the other implementations against each other, so by making this change we are effectively committing to maintaining the equivalent integration scripts (e.g. adding new images and/or keeping up to date with new dependencies or tests).
Maybe this isn't a big deal, but I would prefer not to have to maintain it
cc @viirya
This PR doesn't move away from using archery.py, archery is still used to orchestrate the tests - see here. All it moves away from is the docker-compose CI plumbing within the arrow repo proper, which isn't necessary and is more part of the arrow CI than archery itself... |
| run: conda run --no-capture-output pip install -e dev/archery | ||
| - name: Run integration tests | ||
| run: | | ||
| conda run --no-capture-output archery integration \ |
There was a problem hiding this comment.
Do we need conda run for each commend? I think the docker image enables the conda environment by default?
There was a problem hiding this comment.
Sadly github actions overrides the entrypoint, among other things, and so this is the only way to make it work..
| --gold-dirs=testing/data/arrow-ipc-stream/integration/1.0.0-bigendian \ | ||
| --gold-dirs=testing/data/arrow-ipc-stream/integration/1.0.0-littleendian \ | ||
| --gold-dirs=testing/data/arrow-ipc-stream/integration/2.0.0-compression \ | ||
| --gold-dirs=testing/data/arrow-ipc-stream/integration/4.0.0-shareddict |
There was a problem hiding this comment.
I think the change here is to move executing commends from docker-compose (conda-integration) to separate CI steps. The advantage is easier to control each step for each implementations (e.g. disabling JS).
Although we don't move from archery, there is still a slight disadvantage that we maintain the execution here and if any change is made in arrow repo, we might miss it at first.
There is some overhead but not as much as moving from archery, I think.
There was a problem hiding this comment.
Correct, this should be relatively straightforward to maintain, and we can always revert back should issues arise.
Aside from giving more control, the CI reports will be more helpful as you can see the stage that failed, and it is significantly easier imo to understand what is going on and how to reproduce it locally.
Somewhat related I don't run docker on my machine, instead using podman, and so cannot run the archery docker commands
There was a problem hiding this comment.
Aside from giving more control, the CI reports will be more helpful as you can see the stage that failed, and it is significantly easier imo to understand what is going on and how to reproduce it locally.
Oh, yea, that's right, forgot this point.
|
This looks good to me. Alternatively, if we don't like this approach, we may need to update docker-compose command at arrow repo to disable JS as it doesn't take any environment variables for that. |
|
I think the JS issue has possibly been fixed upstream, but regardless the capability to easily selectively disable broken integration tests still feels valuable, not to mention the other advantages to reproducibility and intelligibility of this PR I'll leave this PR open for now in case anyone else wishes to weigh in, otherwise I'll merge it tomorrow morning |
|
Benchmark runs are scheduled for baseline = a144f69 and contender = 3b59adc. 3b59adc is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
I wonder when we will re-enable the javacript integration test? |
tustvold
added
the
development-process
Which issue does this PR close?
Closes #2448
Rationale for this change
The current integration test has the following flow
This is not only immensely confusing, but makes reproducing issues very difficult and complicates opting out of certain integration tests, e.g. because they are broken #2448
What changes are included in this PR?
Are there any user-facing changes?