refactor(amber): Remove system-requirements-lock.txt #5613
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5613 +/- ##
=========================================
Coverage 53.02% 53.02%
+ Complexity 2657 2652 -5
=========================================
Files 1094 1094
Lines 42286 42319 +33
Branches 4541 4551 +10
=========================================
+ Hits 22423 22441 +18
- Misses 18554 18560 +6
- Partials 1309 1318 +9
*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:
|
Yicong-Huang
left a comment
There was a problem hiding this comment.
Left comments in line
|
@SarahAsad23 I suggest you split the UI change to a separate task. |
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 365 | 0.223 | 24,404/40,182/40,182 us | 🔴 +42.4% / 🔴 +14.9% |
| 🔴 | bs=100 sw=10 sl=64 | 815 | 0.497 | 121,549/144,326/144,326 us | 🔴 +9.3% / 🔴 -8.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 933 | 0.569 | 1,066,166/1,118,471/1,118,471 us | ⚪ within ±5% / 🔴 -10.4% |
Baseline details
Latest main 33903e1 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 365 tuples/sec | 441 tuples/sec | 410.82 tuples/sec | -17.2% | -11.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.223 MB/s | 0.269 MB/s | 0.251 MB/s | -17.1% | -11.1% |
| bs=10 sw=10 sl=64 | p50 | 24,404 us | 22,850 us | 23,785 us | +6.8% | +2.6% |
| bs=10 sw=10 sl=64 | p95 | 40,182 us | 28,225 us | 34,980 us | +42.4% | +14.9% |
| bs=10 sw=10 sl=64 | p99 | 40,182 us | 28,225 us | 34,980 us | +42.4% | +14.9% |
| bs=100 sw=10 sl=64 | throughput | 815 tuples/sec | 844 tuples/sec | 891.94 tuples/sec | -3.4% | -8.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.497 MB/s | 0.515 MB/s | 0.544 MB/s | -3.5% | -8.7% |
| bs=100 sw=10 sl=64 | p50 | 121,549 us | 116,333 us | 112,277 us | +4.5% | +8.3% |
| bs=100 sw=10 sl=64 | p95 | 144,326 us | 132,062 us | 139,802 us | +9.3% | +3.2% |
| bs=100 sw=10 sl=64 | p99 | 144,326 us | 132,062 us | 139,802 us | +9.3% | +3.2% |
| bs=1000 sw=10 sl=64 | throughput | 933 tuples/sec | 945 tuples/sec | 1,041 tuples/sec | -1.3% | -10.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.569 MB/s | 0.577 MB/s | 0.635 MB/s | -1.4% | -10.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,066,166 us | 1,060,019 us | 972,714 us | +0.6% | +9.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,118,471 us | 1,129,848 us | 1,023,057 us | -1.0% | +9.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,118,471 us | 1,129,848 us | 1,023,057 us | -1.0% | +9.3% |
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,547.55,200,128000,365,0.223,24403.85,40181.78,40181.78
1,100,10,64,20,2454.99,2000,1280000,815,0.497,121549.31,144326.29,144326.29
2,1000,10,64,20,21445.91,20000,12800000,933,0.569,1066166.06,1118470.76,1118470.76|
cc @xuang7 for v1.2 backport |
|
I removed the backport label since this will need to be manually backported after merge. Could you resolve the merge conflict? Thanks! @SarahAsad23 |
|
This is more of a refactor PR than a fix. I suggest we do not backport to v1.2 |
# Conflicts: # amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala # amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala
|
@Yicong-Huang Is this PR ready to be merged? |
There was a problem hiding this comment.
Pull request overview
This PR removes the checked-in system-requirements-lock.txt mechanism for PVE system dependency conflict detection, and replaces it with runtime resolution of the fully pinned system dependency set (via a temporary venv + pip freeze). It also simplifies PVE APIs by removing the prior isLocal flag and standardizing how requirements.txt is located.
Changes:
- Delete
amber/system-requirements-lock.txtand switch to runtime-generated system package constraints. - Update PVE REST/WebSocket and tests to use the new
PveManagermethod signatures (noisLocal). - Add runtime caching of resolved system packages + a generated constraints file for
pip install --constraint.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| amber/system-requirements-lock.txt | Removed the checked-in lock file. |
| amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala | Updated tests to match new PveManager signatures. |
| amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala | Removed Kubernetes/local branching and updated calls to PveManager. |
| amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala | Updated REST endpoints to call the new PveManager APIs (no isLocal). |
| amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala | Added runtime system-package resolution (venv + pip freeze), caching, and updated install/delete logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala # amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala # amber/system-requirements-lock.txt
|
@Yicong-Huang All comments have been addressed. |
<!-- Thanks for sending a pull request (PR)! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: [Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md) 2. Ensure you have added or run the appropriate tests for your PR 3. If the PR is work in progress, mark it a draft on GitHub. 4. Please write your PR title to summarize what this PR proposes, we are following Conventional Commits style for PR titles as well. 5. Be sure to keep the PR description updated to reflect all changes. --> ### What changes were proposed in this PR? <!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes. Here are some tips for you: 1. If you propose a new API, clarify the use case for a new API. 2. If you fix a bug, you can clarify why it is a bug. 3. If it is a refactoring, clarify what has been changed. 3. It would be helpful to include a before-and-after comparison using screenshots or GIFs. 4. Please consider writing useful notes for better and faster reviews. --> PR apache#4902 previously introduced a checked-in `system-requirements-lock.txt` file to detect version conflicts between user requested packages and Texera's system dependencies. This file contained a fully resolved dependency list, including transitive dependencies that are not present in `requirements.txt`. Maintaining a static lock file is problematic because dependency resolution is environment dependent and time-dependent as discussed in issue apache#5034. This PR removes the dependency on a static `system-requirements-lock.txt` file and generates the resolved system dependency list dynamically. When the PVE configuration modal is opened for the first time: 1. A temporary Python virtual environment is created. 2. Texera's system requirements (`requirements.txt`) are installed into the temporary environment. 3. pip freeze is executed to capture the fully resolved dependency set, including all transitive dependencies. 4. The generated dependency list is used as the source of truth for package conflict detection. This ensures that conflict checks are performed against the actual dependency versions resolved in the current environment rather than against a potentially stale lock file. ### Any related issues, documentation, discussions? <!-- Please use this section to link other resources if not mentioned already. 1. If this PR fixes an issue, please include `Fixes apache#1234`, `Resolves apache#1234` or `Closes apache#1234`. If it is only related, simply mention the issue number. 2. If there is design documentation, please add the link. 3. If there is a discussion in the mailing list, please add the link. --> Related PR: apache#4902 and related Issue: apache#5034. Closes apache#5608. ### How was this PR tested? <!-- If tests were added, say they were added here. Or simply mention that if the PR is tested with existing test cases. Make sure to include/update test cases that check the changes thoroughly including negative and positive cases if possible. If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future. If tests were not added, please describe why they were not added and/or why it was difficult to add. --> Tested manually and tests exist in `PveResourceSpec.scala` ### Was this PR authored or co-authored using generative AI tooling? <!-- If generative AI tooling has been used in the process of authoring this PR, please include the phrase: 'Generated-by: ' followed by the name of the tool and its version. If no, write 'No'. Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details. --> Co-authored using: Claude Code
…em packages resolve (apache#5644) <!-- Thanks for sending a pull request (PR)! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: [Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md) 2. Ensure you have added or run the appropriate tests for your PR 3. If the PR is work in progress, mark it a draft on GitHub. 4. Please write your PR title to summarize what this PR proposes, we are following Conventional Commits style for PR titles as well. 5. Be sure to keep the PR description updated to reflect all changes. --> ### What changes were proposed in this PR? <!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes. Here are some tips for you: 1. If you propose a new API, clarify the use case for a new API. 2. If you fix a bug, you can clarify why it is a bug. 3. If it is a refactoring, clarify what has been changed. 3. It would be helpful to include a before-and-after comparison using screenshots or GIFs. 4. Please consider writing useful notes for better and faster reviews. --> PR apache#5613 removed the `system-requirements-lock.txt` file which changes how system packages are loaded. Previously, the package list was available immediately from the lock file. With the new approach, the system package set is generated on demand by creating a temporary environment, installing the requirements, and running pip freeze, which introduces a noticeable delay on the initial load. The PR introduces new UI changes to handle this new loading state and provide feedback to the user while the system packages are being resolved by adding a loading spinner. <img width="1948" height="948" alt="image" src="https://github.com/user-attachments/assets/4c80f93c-fbb2-42f0-91dc-4942ccaf67c5" /> <img width="1932" height="1186" alt="image" src="https://github.com/user-attachments/assets/5bde688b-baf9-4151-825e-309893ff8c3d" /> ### Any related issues, documentation, discussions? <!-- Please use this section to link other resources if not mentioned already. 1. If this PR fixes an issue, please include `Fixes apache#1234`, `Resolves apache#1234` or `Closes apache#1234`. If it is only related, simply mention the issue number. 2. If there is design documentation, please add the link. 3. If there is a discussion in the mailing list, please add the link. --> Related PR: apache#5613. Closes apache#5926 ### How was this PR tested? <!-- If tests were added, say they were added here. Or simply mention that if the PR is tested with existing test cases. Make sure to include/update test cases that check the changes thoroughly including negative and positive cases if possible. If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future. If tests were not added, please describe why they were not added and/or why it was difficult to add. --> Tested manually. ### Was this PR authored or co-authored using generative AI tooling? <!-- If generative AI tooling has been used in the process of authoring this PR, please include the phrase: 'Generated-by: ' followed by the name of the tool and its version. If no, write 'No'. Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details. --> Co-authored using: Claude Code
What changes were proposed in this PR?
PR #4902 previously introduced a checked-in
system-requirements-lock.txtfile to detect version conflicts between user requested packages and Texera's system dependencies. This file contained a fully resolved dependency list, including transitive dependencies that are not present inrequirements.txt. Maintaining a static lock file is problematic because dependency resolution is environment dependent and time-dependent as discussed in issue #5034.This PR removes the dependency on a static
system-requirements-lock.txtfile and generates the resolved system dependency list dynamically.When the PVE configuration modal is opened for the first time:
requirements.txt) are installed into the temporary environment.This ensures that conflict checks are performed against the actual dependency versions resolved in the current environment rather than against a potentially stale lock file.
Any related issues, documentation, discussions?
Related PR: #4902 and related Issue: #5034. Closes #5608.
How was this PR tested?
Tested manually and tests exist in
PveResourceSpec.scalaWas this PR authored or co-authored using generative AI tooling?
Co-authored using: Claude Code