chore(amber): move test-only deps to dev-requirements.txt#5692
Open
Yicong-Huang wants to merge 2 commits into
Open
chore(amber): move test-only deps to dev-requirements.txt#5692Yicong-Huang wants to merge 2 commits into
Yicong-Huang wants to merge 2 commits into
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
github-actions
Bot
added
dependencies
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5692 +/- ##
=========================================
Coverage 52.79% 52.79%
Complexity 2548 2548
=========================================
Files 1090 1090
Lines 42150 42150
Branches 4529 4529
=========================================
Hits 22253 22253
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:
|
Contributor
✅ No material benchmark regressions detected🟢 0 better · 🔴 0 worse · ⚪ 15 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_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,526.13,200,128000,380,0.232,24871.29,35255.68,35255.68
1,100,10,64,20,2391.98,2000,1280000,836,0.510,117271.68,150417.58,150417.58
2,1000,10,64,20,21556.66,20000,12800000,928,0.566,1077608.33,1124396.40,1124396.40 |
pluggy 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.
Contributor
Author
|
cc @SarahAsad23 PTAL |
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
2 participants
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?
amber/dev-requirements.txtalready documents the contract: "Test- and dev-only Python dependencies. Installed in CI after the LICENSE-binary snapshot is taken, so packages here never appear in pip-licenses output and never need to be tracked in LICENSE-binary / NOTICE-binary. Not installed by packaging."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-pythonDiff:
4 files changed, 6 insertions(+), 12 deletions(-).Why 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])