test(amber): move state-mat e2e into amber-integration#5682
Open
Yicong-Huang wants to merge 7 commits into
Open
test(amber): move state-mat e2e into amber-integration#5682Yicong-Huang wants to merge 7 commits into
Yicong-Huang wants to merge 7 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5682 +/- ##
============================================
- Coverage 53.09% 52.88% -0.22%
- Complexity 2546 2620 +74
============================================
Files 1076 1090 +14
Lines 41762 42204 +442
Branches 4513 4530 +17
============================================
+ Hits 22174 22318 +144
- Misses 18278 18580 +302
+ Partials 1310 1306 -4
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 412 | 0.252 | 21,405/46,815/46,815 us | 🔴 +89.8% / 🔴 +33.8% |
| 🔴 | bs=100 sw=10 sl=64 | 929 | 0.567 | 103,240/132,706/132,706 us | 🔴 +10.4% / 🟢 -8.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,075 | 0.656 | 926,393/967,467/967,467 us | ⚪ within ±5% / 🟢 -5.4% |
Baseline details
Latest main 6becb85 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 412 tuples/sec | 486 tuples/sec | 410.82 tuples/sec | -15.2% | +0.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.252 MB/s | 0.296 MB/s | 0.251 MB/s | -14.9% | +0.5% |
| bs=10 sw=10 sl=64 | p50 | 21,405 us | 20,930 us | 23,785 us | +2.3% | -10.0% |
| bs=10 sw=10 sl=64 | p95 | 46,815 us | 24,661 us | 34,980 us | +89.8% | +33.8% |
| bs=10 sw=10 sl=64 | p99 | 46,815 us | 24,661 us | 34,980 us | +89.8% | +33.8% |
| bs=100 sw=10 sl=64 | throughput | 929 tuples/sec | 972 tuples/sec | 891.94 tuples/sec | -4.4% | +4.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.567 MB/s | 0.593 MB/s | 0.544 MB/s | -4.4% | +4.2% |
| bs=100 sw=10 sl=64 | p50 | 103,240 us | 100,444 us | 112,277 us | +2.8% | -8.0% |
| bs=100 sw=10 sl=64 | p95 | 132,706 us | 120,216 us | 139,802 us | +10.4% | -5.1% |
| bs=100 sw=10 sl=64 | p99 | 132,706 us | 120,216 us | 139,802 us | +10.4% | -5.1% |
| bs=1000 sw=10 sl=64 | throughput | 1,075 tuples/sec | 1,088 tuples/sec | 1,041 tuples/sec | -1.2% | +3.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.656 MB/s | 0.664 MB/s | 0.635 MB/s | -1.2% | +3.2% |
| bs=1000 sw=10 sl=64 | p50 | 926,393 us | 911,571 us | 972,714 us | +1.6% | -4.8% |
| bs=1000 sw=10 sl=64 | p95 | 967,467 us | 959,226 us | 1,023,057 us | +0.9% | -5.4% |
| bs=1000 sw=10 sl=64 | p99 | 967,467 us | 959,226 us | 1,023,057 us | +0.9% | -5.4% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,485.24,200,128000,412,0.252,21405.17,46815.30,46815.30
1,100,10,64,20,2152.01,2000,1280000,929,0.567,103239.52,132706.10,132706.10
2,1000,10,64,20,18606.66,20000,12800000,1075,0.656,926393.28,967467.48,967467.48
Yicong-Huang
force-pushed
the
test/state-mat-into-amber-integration
branch
3 times, most recently
from
b9a13f2 to
9f13e26
Compare
Drop the macOS-only `pyamber-state-materialization-mac` diagnostic job and the sqlite-backed `SqlCatalog` override it relied on. Mark the two e2e tests with @pytest.mark.integration so they run in the `amber-integration` ubuntu job, which already provisions postgres + iceberg catalog DB + MinIO + Lakekeeper and runs `pytest -m integration` as its last step. Catalog initialization now mirrors test_iceberg_document.py:45 — postgres-backed JdbcCatalog with a tempdir warehouse — so we exercise the real prod catalog code path instead of an sqlite shim. The `StorageConfig.initialize` call is wrapped in a session-autouse fixture (rather than at module import) so it co-exists with test_iceberg_document.py's import-time initialize regardless of pytest collection order. The unit-style `test_process_start_channel_persists_produce_state_on_start_output` that the mac job also ran is unaffected: it monkeypatches the output manager and is already picked up by the regular `pyamber` job's `pytest -m "not integration"` step. Closes apache#5681
Yicong-Huang
force-pushed
the
test/state-mat-into-amber-integration
branch
from
9f13e26 to
009d9e4
Compare
Signed-off-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com>
The macos-latest matrix entry was failing at "Set up job" / "docker:
command not found" because GitHub-hosted macOS runners have no
Docker and `services:` containers are Linux-only. Provision the same
stack natively on macOS:
* postgres@16 via brew, with a `postgres` superuser created so the
psql steps stay OS-agnostic;
* minio via `brew install minio`, backgrounded with nohup;
* lakekeeper from the upstream aarch64-apple-darwin tarball
(published v0.11.0 onward — same version as the Linux docker tag);
* minio-mc via brew for the warehouse bucket-init step;
* protoc release asset switched on \$RUNNER_OS.
The job-level `services.postgres` block is also removed since it's a
Linux-only feature; both branches now start postgres via a step.
Each docker-dependent step branches on \$RUNNER_OS inside its `run:`
script so the YAML keeps one step per concept rather than
Linux/macOS pairs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
protoc 3.19.4 predates upstream arm64-mac builds — the previous osx-aarch_64 URL 404'd. GitHub's Apple Silicon macOS runners ship Rosetta 2 preinstalled, so the x86_64 binary runs transparently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aglinxinyuan
left a comment
aglinxinyuan
left a comment
Contributor
There was a problem hiding this comment.
The change is correct. I will review it after the CI passes.
protoc's import resolver walks each -I in order. With `-I=/usr/local/include` listed first, ambermessage.proto's `import "org/apache/.../controlcommands.proto"` caused protoc to stat `/usr/local/include/org/apache/...`. On Linux that path returns ENOENT and protoc falls through to the next -I; on macOS the unzip-created /usr/local/include lacks +x for the runner user, so the stat returns EACCES and protoc aborts. Widen the existing chmod from `/usr/local/include/google` to `/usr/local/include` so the parent dir is traversable on both OSes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The macOS run dies with `protoc-gen-python_betterproto: Plugin failed with status code 1` and no plugin stderr is surfaced. Wrap the script so a failure dumps python/plugin paths, attempts to import betterproto, and reports protoc's arch — enough to tell whether the plugin is missing, mis-imported, or hitting an arch mismatch. Remove this once the macOS path is stable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rch mismatch
Diagnostics from the last run revealed:
* /usr/local/bin/protoc was Mach-O x86_64 (running under Rosetta);
* protoc-gen-python_betterproto lived in
/Users/runner/hostedtoolcache/Python/3.11.9/arm64/bin/ (arm64-only
setup-python);
* the actual betterproto package was installed into the universal
Framework Python at /Library/Frameworks/Python.framework/...
The x86_64-under-Rosetta protoc could exec the arm64 plugin shim, but
the arch/site-packages split meant the import inside the plugin landed
in a Python that didn't have betterproto wired up the same way, hence
"Plugin failed with status code 1" with no stderr surfaced.
Switching macOS to brew's arm64-native protobuf keeps the protoc /
plugin / python triad on a single arch (arm64), sidestepping all of
this. The version drifts from the pinned 3.19.4 used on Linux, but
python_betterproto's output for proto3 sources is determined by the
betterproto package version, not protoc — so the python codegen
stays bit-identical across the two paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
@aglinxinyuan CI looks green now. Could you please check again? |
aglinxinyuan
approved these changes
Jun 14, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the Python state-materialization end-to-end tests to run as part of the amber-integration CI job (instead of a separate macOS-only diagnostic job), and updates the test setup to exercise the production-like Iceberg catalog path.
Changes:
- Updated
test_state_materialization_e2e.pyto use a postgres-backed IcebergJdbcCatalogpath, added@pytest.mark.integration, and moved initialization into an autouse fixture. - Refactored
amber-integrationin.github/workflows/build.ymlto provision Postgres/MinIO/Lakekeeper on both Linux and macOS runners, and removed the dedicatedpyamber-state-materialization-macjob.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| amber/src/test/python/core/architecture/packaging/test_state_materialization_e2e.py | Switches e2e test from sqlite catalog override to postgres-backed catalog and marks it as integration. |
| .github/workflows/build.yml | Removes the old macOS diagnostic job and updates amber-integration infra provisioning / OS matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
+132
| s3_endpoint="http://localhost:9000", | ||
| s3_region="us-east-1", | ||
| s3_auth_username="minioadmin", | ||
| s3_auth_password="minioadmin", |
Comment on lines
+79
to
+80
| @pytest.fixture(autouse=True) | ||
| def _init_storage_config(self): |
Comment on lines
325
to
327
| matrix: | ||
| os: [ubuntu-22.04] | ||
| os: [ubuntu-22.04, macos-latest] | ||
| java-version: [17] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Drops the macOS-only
pyamber-state-materialization-macdiagnostic job and the sqlite-backedSqlCatalogoverride it relied on. The two state-materialization e2e tests now run under the existingamber-integrationubuntu job, which already provisions postgres + iceberg catalog DB + MinIO + Lakekeeper and runspytest -m integrationas its last step.Concretely:
pyamber-state-materialization-macmacOS job (build.yml:742)SqlCatalog(sqlite) injected in module fixtureJdbcCatalog, matchingtest_iceberg_document.py:45pytest -sv <path>from that job@pytest.mark.integration+ picked up byamber-integration'spytest -m integrationStorageConfig.initializeis wrapped in a session-autouse fixture (rather than called at module import time) so it co-exists withtest_iceberg_document.py's import-timeinitializeregardless of pytest collection order.The unit-style
test_process_start_channel_persists_produce_state_on_start_outputthat the mac job also ran is untouched: it monkeypatches the output manager and is already picked up by the regularpyamberjob'spytest -m "not integration"step.Any related issues, documentation, discussions?
Closes #5681.
How was this PR tested?
Locally against the existing texera-dev infra (postgres on 5432 with
texera_iceberg_catalogschema initialized viasql/iceberg_postgres_catalog.sql):And the regular pyamber suite still green:
In CI, the
amber-integrationjob picks these tests up automatically because they're marked@pytest.mark.integration. The mac job is gone so PRs no longer pay for a separate macOS runner just for these two tests.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)