kunwp1 commented on code in PR #4902:
URL: https://github.com/apache/texera/pull/4902#discussion_r3183392014
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -121,18 +127,6 @@ object PveManager {
return
}
- if (!Files.exists(requirementsPath)) {
- queue.put(s"[PVE][ERR] requirements.txt not found at
${requirementsPath.toAbsolutePath}")
- return
- }
-
- if (!Files.exists(operatorRequirementsPath)) {
- queue.put(
- s"[PVE][ERR] operator-requirements.txt not found at
${operatorRequirementsPath.toAbsolutePath}"
- )
- return
- }
-
Review Comment:
Are these changes necessary in this PR?
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -212,4 +227,91 @@ object PveManager {
stream.close()
}
}
+
+ /**
+ * Installs user requested Python packages into the PVE.
+ *
+ * 1. Executes pip install for each package
+ * 2. Updates user metadata file
+ * 3. Streams logs back via queue
+ */
+ def installUserPackages(
+ packages: List[String],
+ cuid: Int,
+ queue: BlockingQueue[String],
+ pveName: String
+ ): Unit = {
+
+ val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString
+ val envVars = pipEnv
+
+ if (!Files.exists(Paths.get(python))) {
+ queue.put(s"[PVE][ERR] Python executable not found for PVE: $python")
+ return
+ }
+
+ val metadataPath = cuidDir(cuid, pveName).resolve("user-packages.txt")
+ Files.createDirectories(metadataPath.getParent)
Review Comment:
Do we need this line? Can we safely assume that at this point, the pve
folder exists? We already check the existence right above this.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -212,4 +227,91 @@ object PveManager {
stream.close()
}
}
+
+ /**
+ * Installs user requested Python packages into the PVE.
+ *
+ * 1. Executes pip install for each package
+ * 2. Updates user metadata file
+ * 3. Streams logs back via queue
+ */
+ def installUserPackages(
+ packages: List[String],
+ cuid: Int,
+ queue: BlockingQueue[String],
+ pveName: String
+ ): Unit = {
+
+ val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString
+ val envVars = pipEnv
+
+ if (!Files.exists(Paths.get(python))) {
+ queue.put(s"[PVE][ERR] Python executable not found for PVE: $python")
+ return
+ }
+
+ val metadataPath = cuidDir(cuid, pveName).resolve("user-packages.txt")
+ Files.createDirectories(metadataPath.getParent)
+
+ var installedPackages =
+ if (Files.exists(metadataPath)) {
+ Files
+ .readAllLines(metadataPath)
+ .asScala
+ .map(_.trim)
+ .filter(_.nonEmpty)
+ .toSet
+ } else {
+ Set[String]()
+ }
+
+ packages.foreach { pkg =>
+ val trimmedPkg = pkg.trim
+
+ if (trimmedPkg.nonEmpty) {
+ queue.put(s"[PVE] Installing package: $trimmedPkg")
+
+ val code = Process(
+ Seq(
+ python,
+ "-u",
+ "-m",
+ "pip",
+ "install",
+ "--progress-bar",
+ "off",
+ "--no-input",
+ trimmedPkg
+ ),
+ None,
+ envVars.toSeq: _*
+ ).!(
+ ProcessLogger(
+ out => queue.put(s"[pip] $out"),
+ err => queue.put(s"[pip][ERR] $err")
+ )
+ )
+
+ queue.put(s"[pip] install($trimmedPkg) finished with exit code $code")
+
+ if (code != 0) {
+ queue.put(s"[PVE][ERR] Failed to install package: $trimmedPkg")
+ return
+ }
+
+ installedPackages = installedPackages + trimmedPkg
Review Comment:
Better to refactor this logic because I see the same logic when installing
system packages.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html:
##########
Review Comment:
<img width="934" height="480" alt="Image"
src="https://github.com/user-attachments/assets/0c79d91c-3e59-437a-b44c-531ddf4d9d1f";
/>
Can we make make the vertical space between the packages to be consistent
with the system packages?
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -936,13 +933,74 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.cdr.detectChanges();
});
};
+ }
+
+ private refreshUserPackages(index: number): void {
+ const env = this.pves[index];
+
+ this.workflowPveService
+ .getUserPackages(this.selectedComputingUnit!.computingUnit.cuid,
env.name)
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: pkgs => {
+ env.userPackages = pkgs.map(pkg => {
+ const [name, version] = pkg.split("==");
+ return {
+ name: name.trim(),
+ operator: "==",
+ version: (version ?? "").trim(),
+ };
+ });
- socket.onclose = event => {
Review Comment:
Revert this change?
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -936,13 +933,74 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.cdr.detectChanges();
});
};
+ }
+
+ private refreshUserPackages(index: number): void {
+ const env = this.pves[index];
+
+ this.workflowPveService
+ .getUserPackages(this.selectedComputingUnit!.computingUnit.cuid,
env.name)
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: pkgs => {
+ env.userPackages = pkgs.map(pkg => {
+ const [name, version] = pkg.split("==");
+ return {
+ name: name.trim(),
+ operator: "==",
+ version: (version ?? "").trim(),
+ };
+ });
Review Comment:
I see a duplicate logic above. Can you refactor this code?
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -32,7 +32,8 @@ import org.apache.texera.amber.config.PythonUtils
* for each Computing Unit
*
* It supports:
- * - Creating and initializing isolated Python environments
+ * - Creating and initializing isolated Python environments (with system
packages)
+ * - installing user defined packages
Review Comment:
`Installing` should be correct to make the comments consistent.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html:
##########
Review Comment:
Can we add a duplication check so that a user cannot install the same
packages multiple time? Also, I see that a user currently can install system
packages with different versions, but can we disallow this to keep our system
packages freeze?
<img width="943" height="323" alt="Image"
src="https://github.com/user-attachments/assets/cc4a9e8b-ad1b-485e-9e07-ef2db4eaaff8";
/>
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -77,8 +77,16 @@ import { NzSliderComponent } from "ng-zorro-antd/slider";
import { NzAlertComponent } from "ng-zorro-antd/alert";
import { NzCollapseComponent, NzCollapsePanelComponent } from
"ng-zorro-antd/collapse";
+type PackageRow = {
+ name: string;
+ operator?: "==" | ">=" | "<=";
Review Comment:
Maybe name it differently because it's confusing with our texera workflow
operator.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html:
##########
@@ -528,6 +528,88 @@
[(ngModel)]="pve.name"
[disabled]="pve.isLocked || pve.isInstalling" />
</div>
+
+ <!-- USER PACKAGES -->
+ <div
+ *ngFor="let pkg of pve.userPackages; let i = index"
+ class="package-row">
+ <div class="user-package-inputs">
+ <div class="field">
+ <input
+ nz-input
+ [ngModel]="pkg.name"
+ [disabled]="true" />
Review Comment:
Do we have to disable these fields? If a user wants to update the version of
a package, what's our current design of a user experience?
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -758,6 +773,15 @@ export class ComputingUnitSelectionComponent implements
OnInit {
next: (resp: PvePackageResponse[]) => {
this.pves = resp.map(pve => ({
name: pve.pveName,
+ userPackages: pve.userPackages.map(pkgStr => {
+ const [name, version] = pkgStr.split("==");
+ return {
+ name: name.trim(),
+ operator: "==" as const,
+ version: (version ?? "").trim(),
Review Comment:
I don't know how easy it is but if the user doesn't specify any versions,
can we show what version is installed in the UI instead of keeping it a blank?
##########
frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.ts:
##########
@@ -59,11 +59,15 @@ export class WorkflowPveService {
return this.http.get<PvePackageResponse[]>("/pve/pves", { params });
}
+ getUserPackages(cuid: number, pveName: string): Observable<string[]> {
+ return this.fetchPVEs(cuid).pipe(map(pves => pves.find(pve => pve.pveName
=== pveName)?.userPackages ?? []));
+ }
+
deleteEnvironments(cuid: number) {
return this.http.delete(`/pve/pves/${cuid}`);
}
- createPveWebSocketUrl(cuid: number, pveName: string, isLocal: boolean,
packages: string[] = []): string {
+ PveWebSocketUrl(cuid: number, pveName: string, isLocal: boolean, action:
string, packages: string[] = []): string {
Review Comment:
Rename it to `getPveWebSocketUrl`?
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -917,9 +916,7 @@ export class ComputingUnitSelectionComponent implements
OnInit {
});
};
- socket.onerror = err => {
- console.log("PVE WS error", err);
Review Comment:
Revert this change?
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -874,8 +887,6 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.scrollToBottomOfPipModal(index);
socket.onmessage = event => {
- console.log("PVE WS received:", event.data);
Review Comment:
Revert this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
