Skip to content

Commit df23815

Browse files
SarahAsad23kunwp1
SarahAsad23
and
kunwp1
authored
feat: Add Python Virtual Environment Support: Uninstalling User Defined Packages (#5035)
<!-- 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. --> This PR is an extension of PR #4484 and #4902. Previously, we introduced support for creating Python Virtual Environments (PVEs) with system-level dependencies preinstalled, along with support for installing user-defined packages. This PR extends that functionality by allowing users to uninstall user-installed packages from their PVEs. ### 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 #1234`, `Resolves #1234` or `Closes #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. --> This change is part of ongoing efforts to support environment isolation and reproducibility within Texera. Related issue includes #4296. This PR closes sub-issue #4466. ### 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 PveResourceSpec test file updated. To test: On CU click "+" Python Environments. Input environment name. Input package name and version. Click "OK" and wait for pip logs. To delete click on "Delete Icon" and click "OK" ### 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: ChatGPT (OpenAI) --------- Co-authored-by: Kunwoo (Chris) <143021053+kunwp1@users.noreply.github.com>
1 parent ed2f775 commit df23815

7 files changed

Lines changed: 320 additions & 31 deletions

File tree

‎amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,4 +341,97 @@ object PveManager {
341341
queue.put(s"[user-package] $pkg")
342342
}
343343
}
344+
345+
/**
346+
* Uninstalls a user-installed package from the PVE.
347+
* 1. Prevents deletion of system packages
348+
* 2. Updates user metadata upon success
349+
* 3. Returns status messages
350+
*/
351+
def deletePackages(
352+
cuid: Int,
353+
packageName: String,
354+
pveName: String,
355+
isLocal: Boolean
356+
): List[String] = {
357+
val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString
358+
val metadataPath = cuidDir(cuid, pveName).resolve("user-packages.txt")
359+
360+
if (!Files.exists(Paths.get(python))) {
361+
val msg = s"[PVE][ERR] Python executable not found for PVE: $python"
362+
println(msg)
363+
return List(msg)
364+
}
365+
366+
val trimmedPackageName = packageName.trim
367+
val normalizedPackageName = trimmedPackageName.split("==")(0).trim.toLowerCase
368+
369+
val systemPackages =
370+
if (Files.exists(getSystemPath(isLocal))) {
371+
Files
372+
.readAllLines(getSystemPath(isLocal))
373+
.asScala
374+
.map(_.trim)
375+
.filter(line => line.nonEmpty && !line.startsWith("#"))
376+
.map(line => line.split("==")(0).trim.toLowerCase)
377+
.toSet
378+
} else {
379+
Set[String]()
380+
}
381+
382+
if (systemPackages.contains(normalizedPackageName)) {
383+
return List(
384+
s"[PVE][ERR] $trimmedPackageName is a system package and cannot be deleted."
385+
)
386+
}
387+
388+
try {
389+
val command = Process(
390+
Seq(
391+
python,
392+
"-u",
393+
"-m",
394+
"pip",
395+
"uninstall",
396+
"-y",
397+
trimmedPackageName
398+
),
399+
None,
400+
pipEnv.toSeq: _*
401+
)
402+
403+
val output = scala.collection.mutable.ListBuffer[String]()
404+
405+
val exitCode = command.!(
406+
ProcessLogger(
407+
out => {
408+
println(s"[pip] $out")
409+
output += s"[pip] $out"
410+
},
411+
err => {
412+
System.err.println(s"[pip][ERR] $err")
413+
output += s"[pip][ERR] $err"
414+
}
415+
)
416+
)
417+
418+
if (exitCode == 0) {
419+
val updatedPackages = readPackageFile(metadataPath)
420+
.filterNot(line => line.split("==")(0).trim.toLowerCase == normalizedPackageName)
421+
.sorted
422+
423+
Files.write(metadataPath, updatedPackages.asJava)
424+
425+
output += s"[pip] uninstall($trimmedPackageName) finished with exit code $exitCode"
426+
output += s"[PVE] Uninstalled $trimmedPackageName successfully"
427+
} else {
428+
output += s"[PVE][ERR] Failed to uninstall package: $trimmedPackageName"
429+
}
430+
431+
output.toList
432+
} catch {
433+
case e: Exception =>
434+
List(s"[PVE][ERR] Failed to delete package for cuid=$cuid: ${e.getMessage}")
435+
}
436+
}
344437
}

‎amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ import javax.ws.rs._
2323
import javax.ws.rs.core.MediaType
2424
import scala.jdk.CollectionConverters._
2525
import java.util
26+
import javax.ws.rs.DELETE
27+
import javax.ws.rs.PathParam
28+
import javax.ws.rs.core.Response
2629

2730
@Path("/pve")
2831
@Consumes(Array(MediaType.APPLICATION_JSON))
@@ -85,4 +88,29 @@ class PveResource {
8588
def deleteEnvironments(@PathParam("cuId") cuid: Int): Unit = {
8689
PveManager.deleteEnvironments(cuid)
8790
}
91+
92+
// --------------------------------------------------
93+
// Delete User Installed Package
94+
// --------------------------------------------------
95+
@DELETE
96+
@Path("/{cuid}/{pveName}/packages/{packageName}")
97+
def deletePackage(
98+
@PathParam("cuid") cuid: Int,
99+
@PathParam("pveName") pveName: String,
100+
@PathParam("packageName") packageName: String,
101+
@QueryParam("isLocal") isLocal: Boolean
102+
): Response = {
103+
val messages = PveManager.deletePackages(
104+
cuid,
105+
packageName,
106+
pveName,
107+
isLocal
108+
)
109+
110+
if (messages.exists(_.contains("[PVE][ERR]"))) {
111+
Response.status(Response.Status.BAD_REQUEST).entity(messages.asJava).build()
112+
} else {
113+
Response.ok(messages.asJava).build()
114+
}
115+
}
88116
}

‎amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,47 @@ class PveResourceSpec extends AnyFlatSpec with Matchers with BeforeAndAfterEach
9898
pve.get.userPackages should contain(packageSpec)
9999
}
100100

101+
"PveManager" should "delete a user package and remove it from the PVE package list" in {
102+
PveManager.createNewPve(testCuid, queue, testPveName, isLocal = true)
103+
104+
val packageName = "colorama"
105+
val packageVersion = "0.4.6"
106+
val packageSpec = s"$packageName==$packageVersion"
107+
108+
queue.clear()
109+
110+
PveManager.installUserPackages(
111+
List(packageSpec),
112+
testCuid,
113+
queue,
114+
testPveName,
115+
isLocal = true
116+
)
117+
118+
PveManager
119+
.getEnvironments(testCuid)
120+
.find(_.pveName == testPveName)
121+
.get
122+
.userPackages should contain(packageSpec)
123+
124+
val deleteLogs = PveManager.deletePackages(
125+
testCuid,
126+
packageName,
127+
testPveName,
128+
isLocal = true
129+
)
130+
131+
deleteLogs.mkString("\n") should not include "[PVE][ERR]"
132+
deleteLogs.mkString("\n") should include(s"[PVE] Uninstalled $packageName successfully")
133+
134+
val pve = PveManager
135+
.getEnvironments(testCuid)
136+
.find(_.pveName == testPveName)
137+
138+
pve should not be empty
139+
pve.get.userPackages should not contain packageSpec
140+
}
141+
101142
"PveManager" should "delete all PVEs for a computing unit" in {
102143
PveManager.createNewPve(testCuid, queue, testPveName, isLocal = true)
103144

‎frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -554,17 +554,31 @@
554554
[ngModel]="pkg.version"
555555
[disabled]="true" />
556556
</div>
557+
<button
558+
nz-button
559+
nzType="default"
560+
nzShape="circle"
561+
nzDanger
562+
[ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"
563+
(click)="togglePackageDelete(envIndex, pkg)">
564+
<i
565+
nz-icon
566+
nzType="delete"></i>
567+
</button>
557568
</div>
558569
</div>
559570

560571
<!-- NEW PACKAGES -->
561572
<div class="new-packages-section">
562573
<div
563-
class="user-package-header-row"
574+
class="package-row user-package-header-row"
564575
*ngIf="pve.newPackages.length > 0">
565-
<div class="package-column-label">Package</div>
566-
<label style="visibility: hidden">Op</label>
567-
<div class="package-column-label">Version</div>
576+
<div class="user-package-inputs">
577+
<div class="package-column-label">Package</div>
578+
<div></div>
579+
<div class="package-column-label">Version</div>
580+
<div></div>
581+
</div>
568582
</div>
569583

570584
<div
@@ -601,6 +615,17 @@
601615
placeholder="Package Version"
602616
[(ngModel)]="pve.newPackages[i].version" />
603617
</div>
618+
<button
619+
nz-button
620+
nzType="default"
621+
nzShape="circle"
622+
nzDanger
623+
[ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"
624+
(click)="togglePackageDelete(envIndex, pkg)">
625+
<i
626+
nz-icon
627+
nzType="delete"></i>
628+
</button>
604629
</div>
605630
</div>
606631
</div>

‎frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts‎

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,12 +81,14 @@ type PveUserPackageRow = {
8181
name: string;
8282
versionOp?: "==" | ">=" | "<=";
8383
version?: string;
84+
deleteToggle?: boolean;
8485
};
8586

8687
type PveDraft = {
8788
name: string;
8889
userPackages: PveUserPackageRow[];
8990
newPackages: PveUserPackageRow[];
91+
deletingPackages: { name: string; version: string }[];
9092
pipOutput: string;
9193
prettyPipOutput: string;
9294
expanded: boolean;
@@ -732,14 +734,29 @@ export class ComputingUnitSelectionComponent implements OnInit {
732734

733735
addPackage(index: number): void {
734736
const env = this.pves[index];
735-
env.newPackages.push({ name: "", version: "", versionOp: undefined });
737+
env.newPackages.push({ name: "", version: "", versionOp: undefined, deleteToggle: false });
738+
}
739+
740+
togglePackageDelete(index: number, pkg: PveUserPackageRow): void {
741+
const env = this.pves[index];
742+
743+
pkg.deleteToggle = !pkg.deleteToggle;
744+
745+
const version = pkg.version ?? "";
746+
747+
env.deletingPackages = env.deletingPackages.filter(p => !(p.name === pkg.name && p.version === version));
748+
749+
if (pkg.deleteToggle) {
750+
env.deletingPackages.push({ name: pkg.name, version });
751+
}
736752
}
737753

738754
addEnvironment(): void {
739755
this.pves.push({
740756
name: "",
741757
userPackages: [],
742758
newPackages: [],
759+
deletingPackages: [],
743760
pipOutput: "",
744761
prettyPipOutput: "",
745762
expanded: true,
@@ -776,6 +793,7 @@ export class ComputingUnitSelectionComponent implements OnInit {
776793
name: pve.pveName,
777794
userPackages: this.parsePackageRows(pve.userPackages),
778795
newPackages: [],
796+
deletingPackages: [],
779797
expanded: false,
780798
isInstalling: false,
781799
pipOutput: "",
@@ -949,14 +967,17 @@ export class ComputingUnitSelectionComponent implements OnInit {
949967
createVirtualEnvironment(index: number): void {
950968
const env = this.pves[index];
951969
const trimmedName = env.name.trim();
970+
const isLocal = this.selectedComputingUnit?.computingUnit.type === "local";
952971

953972
if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) {
954973
this.notificationService.error("Environment name must contain only letters and numbers.");
955974
return;
956975
}
957976

958977
if (env.isLocked) {
959-
this.installUserPackages(index);
978+
this.deleteUserPackages(index, () => {
979+
this.installUserPackages(index);
980+
});
960981
return;
961982
}
962983

@@ -968,7 +989,9 @@ export class ComputingUnitSelectionComponent implements OnInit {
968989
}
969990

970991
this.runPveWebSocket(index, "create", "Creating virtual environment...\n", [], () => {
971-
this.installUserPackages(index);
992+
this.deleteUserPackages(index, () => {
993+
this.installUserPackages(index);
994+
});
972995
});
973996
}
974997

@@ -1019,6 +1042,7 @@ export class ComputingUnitSelectionComponent implements OnInit {
10191042

10201043
if (packageArray.length === 0) {
10211044
this.pves[index].newPackages = [];
1045+
this.pves[index].isInstalling = false;
10221046
this.refreshUserPackages(index);
10231047
return;
10241048
}
@@ -1039,4 +1063,62 @@ export class ComputingUnitSelectionComponent implements OnInit {
10391063
};
10401064
});
10411065
}
1066+
1067+
private deleteUserPackages(index: number, onDone?: () => void): void {
1068+
const cuId = this.selectedComputingUnit!.computingUnit.cuid;
1069+
const isLocal = this.selectedComputingUnit?.computingUnit.type === "local";
1070+
const pveName = this.pves[index].name.trim();
1071+
const packagesToDelete = [...this.pves[index].deletingPackages];
1072+
1073+
if (packagesToDelete.length === 0) {
1074+
onDone?.();
1075+
return;
1076+
}
1077+
1078+
this.pves[index] = {
1079+
...this.pves[index],
1080+
pipOutput: `${this.pves[index].pipOutput ?? ""}Deleting user packages...\n`,
1081+
isInstalling: true,
1082+
};
1083+
1084+
let deleteIndex = 0;
1085+
1086+
const deleteNext = (): void => {
1087+
if (deleteIndex >= packagesToDelete.length) {
1088+
this.pves[index].deletingPackages = [];
1089+
this.refreshUserPackages(index);
1090+
onDone?.();
1091+
return;
1092+
}
1093+
1094+
const pkg = packagesToDelete[deleteIndex];
1095+
1096+
this.workflowPveService
1097+
.deletePackage(cuId, pveName, pkg.name, isLocal)
1098+
.pipe(untilDestroyed(this))
1099+
.subscribe({
1100+
next: messages => {
1101+
this.pves[index].pipOutput = `${this.pves[index].pipOutput ?? ""}${messages.join("\n")}\n`;
1102+
1103+
this.updatePrettyPipOutput(index);
1104+
this.scrollToBottomOfPipModal(index);
1105+
1106+
deleteIndex++;
1107+
deleteNext();
1108+
},
1109+
error: () => {
1110+
this.pves[index].pipOutput =
1111+
`${this.pves[index].pipOutput ?? ""}[PVE][ERR] Failed to delete package: ${pkg.name}\n`;
1112+
1113+
this.updatePrettyPipOutput(index);
1114+
this.scrollToBottomOfPipModal(index);
1115+
1116+
deleteIndex++;
1117+
deleteNext();
1118+
},
1119+
});
1120+
};
1121+
1122+
deleteNext();
1123+
}
10421124
}

0 commit comments

Comments
 (0)