chore(amber): move test-only deps to dev-requirements.txt#5692
Conversation
pytest, pytest-reraise, pytest-timeout, and iniconfig were pinned in amber/requirements.txt despite being test-only. Per the header comment in amber/dev-requirements.txt, test deps belong there so they stay out of the LICENSE-binary snapshot and the production CU Docker images (which install requirements.txt + operator-requirements.txt only). Move pytest, pytest-reraise, pytest-timeout to dev-requirements.txt. Drop iniconfig entirely — it is pytest's transitive and gets pulled in automatically. Remove matching entries from amber/system-requirements-lock.txt (its header says it tracks requirements + operator-requirements) and from amber/LICENSE-binary-python. Closes apache#5691
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5692 +/- ##
============================================
+ Coverage 52.79% 52.82% +0.02%
Complexity 2548 2548
============================================
Files 1090 1090
Lines 42150 42175 +25
Branches 4529 4529
============================================
+ Hits 22253 22278 +25
Misses 18575 18575
Partials 1322 1322
*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:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 387 | 0.236 | 25,078/34,831/34,831 us | 🔴 +16.4% / 🔴 -5.9% |
| ⚪ | bs=100 sw=10 sl=64 | 814 | 0.497 | 120,596/157,608/157,608 us | ⚪ within ±5% / 🔴 +12.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 932 | 0.569 | 1,073,642/1,105,879/1,105,879 us | ⚪ within ±5% / 🔴 -10.5% |
Baseline details
Latest main a044287 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 387 tuples/sec | 440 tuples/sec | 410.82 tuples/sec | -12.0% | -5.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.236 MB/s | 0.268 MB/s | 0.251 MB/s | -11.9% | -5.9% |
| bs=10 sw=10 sl=64 | p50 | 25,078 us | 21,553 us | 23,785 us | +16.4% | +5.4% |
| bs=10 sw=10 sl=64 | p95 | 34,831 us | 32,793 us | 34,980 us | +6.2% | -0.4% |
| bs=10 sw=10 sl=64 | p99 | 34,831 us | 32,793 us | 34,980 us | +6.2% | -0.4% |
| bs=100 sw=10 sl=64 | throughput | 814 tuples/sec | 811 tuples/sec | 891.94 tuples/sec | +0.4% | -8.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.497 MB/s | 0.495 MB/s | 0.544 MB/s | +0.4% | -8.7% |
| bs=100 sw=10 sl=64 | p50 | 120,596 us | 119,826 us | 112,277 us | +0.6% | +7.4% |
| bs=100 sw=10 sl=64 | p95 | 157,608 us | 158,948 us | 139,802 us | -0.8% | +12.7% |
| bs=100 sw=10 sl=64 | p99 | 157,608 us | 158,948 us | 139,802 us | -0.8% | +12.7% |
| bs=1000 sw=10 sl=64 | throughput | 932 tuples/sec | 939 tuples/sec | 1,041 tuples/sec | -0.7% | -10.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.569 MB/s | 0.573 MB/s | 0.635 MB/s | -0.7% | -10.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,073,642 us | 1,062,359 us | 972,714 us | +1.1% | +10.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,105,879 us | 1,136,873 us | 1,023,057 us | -2.7% | +8.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,105,879 us | 1,136,873 us | 1,023,057 us | -2.7% | +8.1% |
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,516.22,200,128000,387,0.236,25077.55,34830.96,34830.96
1,100,10,64,20,2455.50,2000,1280000,814,0.497,120596.39,157607.74,157607.74
2,1000,10,64,20,21467.66,20000,12800000,932,0.569,1073642.25,1105879.20,1105879.20pluggy is a pytest transitive (not directly imported anywhere in the repo); once pytest moves to dev-requirements.txt it is no longer installed during the LICENSE-binary snapshot, so check_binary_deps.py flags it as stale. Drop the matching lines.
|
cc @SarahAsad23 PTAL |
kunwp1
left a comment
There was a problem hiding this comment.
LGTM! Can you update the PR description because it's outdated with LoC changes and also include pluggy?
Per review discussion, transitive dependencies need not be declared even in comments, so remove the iniconfig note from the pytest comment block.
|
thanks, merging. |
What changes were proposed in this PR?
Four test-only pins were sitting in the wrong file (
amber/requirements.txt), which leaked them into the production Docker image and the LICENSE-binary snapshot. This PR moves them.pytest==7.4.0requirements.txt→dev-requirements.txtpytest-reraise==2.1.2requirements.txt→dev-requirements.txtpytest-timeout==2.2.0requirements.txt→dev-requirements.txtiniconfig==1.1.1requirements.txt→ dropped entirely (pytest transitive, auto-installed)Side effects:
amber/system-requirements-lock.txtamber/LICENSE-binary-pythonWhy this is safe
pyamber/amber-integrationstacksrequirements.txt+operator-requirements.txt+dev-requirements.txt— get pytest twicebin/computing-unit-master.dockerfile(prod)requirements.txt+operator-requirements.txt— gets pytest, doesn't need itbin/computing-unit-worker.dockerfile(prod)PveManager.createNewPve(per-user PVEs)requirements.txtonly — gets pytest, doesn't need itcheck_binary_deps.py(LICENSE drift gate)The 1 non-test pytest import I found (
amber/src/main/scala/.../aiassistant/test_type_annotation_visitor.py) is a pytest test that's accidentally placed undersrc/mainby filename convention (test_*); it's not invoked by the production runtime.Any related issues, documentation, discussions?
Closes #5691
How was this PR tested?
git diff upstream/main --stat— confirms only the four files above are touched, with the expected+6/-12shapegrep -ciE "^(pytest|iniconfig| - pytest| - iniconfig)" amber/requirements.txt amber/system-requirements-lock.txt amber/LICENSE-binary-python— all three return 0 after the changegrep -rIE "^(from|import)[[:space:]]+pytest"shows 48 usages inamber/src/test/python(CI installs dev-requirements.txt, so they keep working) and the 1 non-runtime file noted aboveWas this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7 [1M context])