Copilot commented on code in PR #4484: URL: https://github.com/apache/texera/pull/4484#discussion_r3128434488
########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import java.io.{File, RandomAccessFile} +import java.nio.charset.StandardCharsets Review Comment: There are unused imports (`java.io.File`, `java.io.RandomAccessFile`, `java.nio.charset.StandardCharsets`) in this file. With the repo’s unused-import cleanup (scalafix/remove-unused), these should be removed to keep CI formatting/lint passing. ```suggestion ``` ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> Review Comment: Closing the modal only sets `pveModalVisible = false` and doesn’t run any cleanup. If an SSE install stream is active, it can continue running after the modal is dismissed. Call a close/cleanup method from `(nzOnCancel)`/`(nzAfterClose)` to close active `EventSource`s and reset state. ```suggestion (nzOnCancel)="closePveModal()" (nzAfterClose)="closePveModal()" [nzFooter]="customFooter"> <ng-template #customFooter> <div class="footer-all"> <button nz-button nzType="default" (click)="closePveModal()"> ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import java.io.{File, RandomAccessFile} +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths, StandardOpenOption} +import java.util.concurrent.BlockingQueue +import scala.collection.mutable.Map +import scala.jdk.CollectionConverters._ +import scala.sys.process._ + +/** + * PveManager is responsible for managing Python Virtual Environments (PVEs) + * for each Computing Unit + * + * It supports: + * - Creating and initializing isolated Python environments + * - Installing and uninstalling Python packages + * - Tracking system vs user-installed packages via metadata files + * - Streaming pip output logs back to the caller + * + * Each PVE is stored under: + * /tmp/texera-pve/venvs/{cuid}/{pveName}/ + * + * The structure includes: + * - pve/ -> actual virtual environment + * - metadata/ -> package tracking (system + user) + */ + +object PveManager { + + private val VenvRoot: Path = Paths.get("/tmp/texera-pve/venvs") + + private def cuidDir(cuid: Int, pvename: String): Path = { + val dir = VenvRoot.resolve(cuid.toString).resolve(pvename) + Files.createDirectories(dir) + dir + } + + private def pveDir(cuid: Int, pveName: String): Path = + cuidDir(cuid, pveName).resolve("pve") + + private def pythonBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("python") + + private def pipBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("pip") + + private def metadataDir(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("metadata") + + private def systemPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("system-packages.txt") + + private def userPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("user-packages.txt") + + private def writeMetadata(path: Path, lines: Seq[String]): Unit = { + if (path.getParent != null) { + Files.createDirectories(path.getParent) + } + Files.write( + path, + lines.asJava, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE + ) + } + + private def readMetadataList(path: Path): List[String] = { + if (!Files.exists(path)) return Nil + Files.readAllLines(path).asScala.map(_.trim).filter(_.nonEmpty).toList + } + + private def pipEnv: Map[String, String] = + Map( + "PYTHONUNBUFFERED" -> "1", + "PIP_PROGRESS_BAR" -> "off", + "PIP_DISABLE_PIP_VERSION_CHECK" -> "1", + "PIP_NO_INPUT" -> "1" + ) + + def getSystemAndUserPackages(cuid: Int, pveName: String): (Seq[String], Seq[String]) = { + val sys = readMetadataList(systemPackagesPath(cuid, pveName)) + val usr = readMetadataList(userPackagesPath(cuid, pveName)) + (sys, usr) + } + + /** + * Creates a new PVE for a CU. + * + * Behavior: + * - If a base PVE exists (PVE_BASE), it clones it for faster setup + * - Otherwise, creates a fresh venv and installs dependencies + * + * Steps: + * 1. Remove existing environment (if present) + * 2. Create or copy base environment + * 3. Install system + operator dependencies + * 4. Record installed packages as system metadata + * + * Logs progress to the provided queue. + */ + def createNewPve(cuid: Int, queue: BlockingQueue[String], pveName: String): Unit = { + queue.put(s"[PVE] Creating new PVE for cuid=$cuid with name=$pveName") + + val venvDirPath = pveDir(cuid, pveName).toAbsolutePath + val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString + val envVars = pipEnv + + val pveBase = sys.env.getOrElse("PVE_BASE", "/opt/pve-base") + val basePython = Paths.get(pveBase).resolve("bin").resolve("python") + val hasBasePve = Files.exists(basePython) + + val requirementsPath = + if (!hasBasePve) Paths.get("amber", "requirements.txt") + else Paths.get("/tmp", "requirements.txt") + + val operatorRequirementsPath = + if (!hasBasePve) Paths.get("amber", "operator-requirements.txt") + else Paths.get("/tmp", "operator-requirements.txt") + + if (!hasBasePve) { + if (Files.exists(venvDirPath)) { + val rmCode = Process(Seq("bash", "-lc", s"rm -rf '${venvDirPath.toString}'")).!( + ProcessLogger( + out => queue.put(s"[pve] $out"), + err => queue.put(s"[pve][ERR] $err") + ) Review Comment: This removal uses `bash -lc` with an interpolated path that ultimately depends on user-controlled `pveName`. This is vulnerable to command injection and path traversal if `pveName` contains quotes/`..`/shell metacharacters. Validate `pveName` server-side and avoid shell invocation by using argument-safe `Process(Seq(...))` or NIO deletion APIs. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, + @QueryParam("cuid") cuid: Int, Review Comment: This resource doesn’t require authentication/authorization (no `@Auth SessionUser` / role checks). As written, callers can create/list PVEs for arbitrary `cuid` values. Please enforce auth and validate the caller has appropriate access to the specified computing unit before performing any filesystem/process operations. ########## frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.ts: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Injectable } from "@angular/core"; +import { BehaviorSubject, Observable } from "rxjs"; +import { HttpClient, HttpParams } from "@angular/common/http"; + +export interface PackageResponse { + system: string[]; + user: string[]; +} + +export interface PvePackageResponse { + pveName: string; + userPackages: string[]; +} + +@Injectable({ providedIn: "root" }) +export class WorkflowPveService { + private cuidSubject = new BehaviorSubject<number | null>(null); + + private pveNameSubject = new BehaviorSubject<string | null>(null); + + constructor(private http: HttpClient) {} + + setCuid(cuid: number): void { + this.cuidSubject.next(cuid); + } + + setPveName(pveName: string): void { + this.pveNameSubject.next(pveName); + } + + private requireCuid(): number { + const cuid = this.cuidSubject.value; + if (cuid === null) { + throw new Error("cuid is not set"); + } + return cuid; + } + + private requirePveName(): string { + const pveName = this.pveNameSubject.value; + if (pveName === null) { + throw new Error("Environment Name is not set"); + } + + return pveName; + } + + private getAccessToken(): string | null { + const token = localStorage.getItem("access_token"); + return token && token.trim().length > 0 ? token : null; + } Review Comment: This service reads the access token directly from `localStorage` using a hard-coded key. The rest of the frontend typically centralizes token access via `AuthService.getAccessToken()` / `TOKEN_KEY` (e.g., websocket and dataset upload flows). Consider switching to `AuthService` here to avoid duplicated auth logic and keep token handling consistent. ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <!-- NEW PACKAGES --> + <div + *ngFor="let pkg of pve.newPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + placeholder="Package Name" /> + </div> + + <div class="field operator operator-select"> + <label style="visibility: hidden">Op</label> + <nz-select nzPlaceHolder="Select"> + <nz-option + nzValue="==" + nzLabel="=="></nz-option> + <nz-option Review Comment: The operator `<nz-select>` in the “New Packages” row isn’t bound to `pkg.operator` (no `[(ngModel)]`), so the selected constraint operator won’t be reflected in `env.newPackages` when building the install request. ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <!-- NEW PACKAGES --> + <div + *ngFor="let pkg of pve.newPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + placeholder="Package Name" /> + </div> + + <div class="field operator operator-select"> + <label style="visibility: hidden">Op</label> + <nz-select nzPlaceHolder="Select"> + <nz-option + nzValue="==" + nzLabel="=="></nz-option> + <nz-option + nzValue=">=" + nzLabel=">="></nz-option> + <nz-option + nzValue="<=" + nzLabel="<="></nz-option> + </nz-select> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + placeholder="Package Version (optional)" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> Review Comment: The delete button in the “New Packages” rows has no click handler, so users can’t remove an added row (and `deleteToggle` won’t change). Wire it to remove/toggle the row, or hide it for now. ```suggestion ``` ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <!-- NEW PACKAGES --> + <div + *ngFor="let pkg of pve.newPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + placeholder="Package Name" /> Review Comment: The “New Packages” package name input is missing `[(ngModel)]` binding to the corresponding `pkg.name`. Without it, `createVirtualEnvironment()` will always see an empty package name regardless of what the user types. ```suggestion placeholder="Package Name" [(ngModel)]="pkg.name" /> ``` ########## frontend/src/app/workspace/service/virtual-environment/virtual-environment.service.ts: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Injectable } from "@angular/core"; +import { BehaviorSubject, Observable } from "rxjs"; +import { HttpClient, HttpParams } from "@angular/common/http"; + +export interface PackageResponse { + system: string[]; + user: string[]; +} + +export interface PvePackageResponse { + pveName: string; + userPackages: string[]; +} + +@Injectable({ providedIn: "root" }) +export class WorkflowPveService { + private cuidSubject = new BehaviorSubject<number | null>(null); + + private pveNameSubject = new BehaviorSubject<string | null>(null); + + constructor(private http: HttpClient) {} + + setCuid(cuid: number): void { + this.cuidSubject.next(cuid); + } + + setPveName(pveName: string): void { + this.pveNameSubject.next(pveName); + } + + private requireCuid(): number { + const cuid = this.cuidSubject.value; + if (cuid === null) { + throw new Error("cuid is not set"); + } + return cuid; + } + + private requirePveName(): string { + const pveName = this.pveNameSubject.value; + if (pveName === null) { + throw new Error("Environment Name is not set"); + } + + return pveName; + } + + private getAccessToken(): string | null { + const token = localStorage.getItem("access_token"); + return token && token.trim().length > 0 ? token : null; + } + + private buildAuthParams(): HttpParams { + let params = new HttpParams().set("cuid", this.requireCuid().toString()); + const token = this.getAccessToken(); + if (token) { + params = params.set("access-token", token); + } + const pveName = this.requirePveName(); + params = params.set("pveName", pveName); + return params; + } + + private buildBaseParams(): HttpParams { + let params = new HttpParams(); + const token = this.getAccessToken(); + if (token) { + params = params.set("access-token", token); + } + return params; Review Comment: `buildAuthParams()` adds `access-token` to the query string. For normal `HttpClient` calls the app already attaches the JWT via `Authorization: Bearer ...` (JwtModule), so putting the token in the URL is redundant and increases exposure via logs/referrers. Consider removing the query-param token for REST calls (keep it only for `EventSource` where headers can’t be set). ```suggestion private buildAuthParams(): HttpParams { let params = new HttpParams().set("cuid", this.requireCuid().toString()); const pveName = this.requirePveName(); params = params.set("pveName", pveName); return params; } private buildBaseParams(): HttpParams { return new HttpParams(); ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): ChunkedOutput[String] = { + val queue = new LinkedBlockingQueue[String]() + val chunkedOutput = new ChunkedOutput[String](classOf[String]) + + Future { + try { + + if (!PveManager.pveExists(cuid, pveName)) { + PveManager.createNewPve(cuid, queue, pveName) Review Comment: `createNewPve` does blocking IO and external process execution, but it’s executed on `ExecutionContext.Implicits.global`. Use a dedicated Dropwizard-managed executor for blocking work and/or wrap blocking sections in `scala.concurrent.blocking` to avoid starving the global pool. ########## frontend/src/styles.scss: ########## @@ -107,3 +107,252 @@ hr { position: relative; left: 0; } + +// pip modal +.pve-modal { + .ant-modal { + max-width: 980px; + width: 92vw !important; + } + + .ant-modal-body { + padding: 16px 20px 18px; + background: #fafafa; + } + + .ant-modal-header { + padding: 14px 20px; + } + + .ant-modal-title { + font-weight: 600; + letter-spacing: 0.2px; + } + + .footer-all { + display: flex; + justify-content: space-between; + align-items: center; + width: 100%; + gap: 10px; + padding: 8px 0; + } + + nz-collapse { + display: block; + } + + .ant-collapse { + border-radius: 12px; + overflow: hidden; + background: transparent; + } + + .system-section { + margin-bottom: 14px; + } + + .system-section .ant-collapse-item { + //border-radius: 12px; + overflow: hidden; + background: #ffffff; + border: 1px solid #eef0f3; + box-shadow: 0 1px 2px rgba(0, 0, 0, 0.03); + } + + .system-section .ant-collapse-header { + font-weight: 600; + } + + .system-panel-inner { + padding: 6px 0; + } + + .system-row { + opacity: 0.9; + } + + .system-input { + background: #f5f6f8 !important; + border-color: #e6e8ec !important; + color: #5a667a; + cursor: not-allowed; + } + + .env-header { + width: 100%; + display: flex; + align-items: center; + justify-content: space-between; + gap: 12px; + } + + .env-title { + font-weight: 600; + font-size: 14px; + color: #1f2a37; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + + .env-actions { + display: inline-flex; + align-items: center; + gap: 8px; + flex-shrink: 0; + } + + .ant-collapse-item { + background: #ffffff; + border: 1px solid #eef0f3; + //border-radius: 12px; + overflow: hidden; + margin-bottom: 12px; + box-shadow: 0 1px 2px rgba(0, 0, 0, 0.03); + } + + .ant-collapse-header { + padding: 12px 14px !important; + align-items: center !important; + } + + .ant-collapse-content-box { + padding: 14px !important; + } + + .ve-form { + display: flex; + flex-direction: column; + gap: 14px; + } + + .fieldRow { + display: flex !important; + align-items: center !important; + gap: 12px !important; + } + + .fieldLabel { + width: 220px; + margin: 0; + font-weight: 700; + white-space: nowrap; + } + + .fieldInput { + flex: 1; + min-width: 0; + } + + .package-row { + display: flex; + align-items: flex-end; + justify-content: space-between; + gap: 10px; + padding: 10px 10px; + border: 1px solid #eef0f3; + //border-radius: 12px; + background: #ffffff; + } + + .package-inputs { + display: grid; + grid-template-columns: 1fr 140px 1fr; + gap: 10px; + flex: 1; + min-width: 0; + } + + .field { + display: flex; + flex-direction: column; + gap: 6px; + min-width: 0; + } + + .field label { + font-size: 11px; + font-weight: 600; + color: #6b7280; + line-height: 1; + } + + .operator-select .ant-select { + width: 100%; + } + + .ant-input, + .ant-select-selector { + //border-radius: 10px !important; + } + + .ant-input[disabled] { + background: #f5f6f8 !important; + border-color: #e6e8ec !important; + color: #5a667a; + } + + .highlighted-btn { + background-color: #ff4d4f !important; /* Ant Design red */ + border-color: #ff4d4f !important; + color: white !important; + } + + .add-btn { + display: flex; + justify-content: flex-start; + margin-top: -6px; + } + + .env-footer { + display: flex; + justify-content: flex-end; + padding-top: 6px; + } + + .pip-panel { + margin-top: 16px; + border: 1px solid #d9d9d9; + //border-radius: 8px; + background: #f2f2f2; + overflow: hidden; + } + + .pip-panel-header { + display: flex; + justify-content: space-between; + align-items: baseline; + padding: 10px 14px; + background: #e9e9e9; + border-bottom: 1px solid #d9d9d9; + } + + .pip-panel-title { + font-weight: 600; + color: #222; + } + + .pip-panel-subtitle { + font-size: 12px; + color: #666; + } + + .pip-panel-body { + padding: 0; + } + + .pip-fullscreen-log { + color: #333; + font-family: "JetBrains Mono", monospace; + font-size: 13px; + line-height: 1.6; Review Comment: The pip log formatting in TS injects classes like `error`/`warning`/`success` and `pip-exit ok/err`, but there are no corresponding CSS rules under `.pve-modal`. If highlighting is intended, add styles for those classes so the UI changes are visible. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import java.io.{File, RandomAccessFile} +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths, StandardOpenOption} +import java.util.concurrent.BlockingQueue +import scala.collection.mutable.Map +import scala.jdk.CollectionConverters._ +import scala.sys.process._ + +/** + * PveManager is responsible for managing Python Virtual Environments (PVEs) + * for each Computing Unit + * + * It supports: + * - Creating and initializing isolated Python environments + * - Installing and uninstalling Python packages + * - Tracking system vs user-installed packages via metadata files + * - Streaming pip output logs back to the caller + * + * Each PVE is stored under: + * /tmp/texera-pve/venvs/{cuid}/{pveName}/ + * + * The structure includes: + * - pve/ -> actual virtual environment + * - metadata/ -> package tracking (system + user) + */ + +object PveManager { + + private val VenvRoot: Path = Paths.get("/tmp/texera-pve/venvs") + + private def cuidDir(cuid: Int, pvename: String): Path = { + val dir = VenvRoot.resolve(cuid.toString).resolve(pvename) + Files.createDirectories(dir) + dir + } + + private def pveDir(cuid: Int, pveName: String): Path = + cuidDir(cuid, pveName).resolve("pve") + + private def pythonBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("python") + + private def pipBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("pip") + + private def metadataDir(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("metadata") + + private def systemPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("system-packages.txt") + + private def userPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("user-packages.txt") + + private def writeMetadata(path: Path, lines: Seq[String]): Unit = { + if (path.getParent != null) { + Files.createDirectories(path.getParent) + } + Files.write( + path, + lines.asJava, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE + ) + } + + private def readMetadataList(path: Path): List[String] = { + if (!Files.exists(path)) return Nil + Files.readAllLines(path).asScala.map(_.trim).filter(_.nonEmpty).toList + } + + private def pipEnv: Map[String, String] = + Map( + "PYTHONUNBUFFERED" -> "1", + "PIP_PROGRESS_BAR" -> "off", + "PIP_DISABLE_PIP_VERSION_CHECK" -> "1", + "PIP_NO_INPUT" -> "1" + ) + + def getSystemAndUserPackages(cuid: Int, pveName: String): (Seq[String], Seq[String]) = { + val sys = readMetadataList(systemPackagesPath(cuid, pveName)) + val usr = readMetadataList(userPackagesPath(cuid, pveName)) + (sys, usr) + } + + /** + * Creates a new PVE for a CU. + * + * Behavior: + * - If a base PVE exists (PVE_BASE), it clones it for faster setup + * - Otherwise, creates a fresh venv and installs dependencies + * + * Steps: + * 1. Remove existing environment (if present) + * 2. Create or copy base environment + * 3. Install system + operator dependencies + * 4. Record installed packages as system metadata + * + * Logs progress to the provided queue. + */ + def createNewPve(cuid: Int, queue: BlockingQueue[String], pveName: String): Unit = { + queue.put(s"[PVE] Creating new PVE for cuid=$cuid with name=$pveName") + + val venvDirPath = pveDir(cuid, pveName).toAbsolutePath + val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString + val envVars = pipEnv + + val pveBase = sys.env.getOrElse("PVE_BASE", "/opt/pve-base") + val basePython = Paths.get(pveBase).resolve("bin").resolve("python") + val hasBasePve = Files.exists(basePython) + + val requirementsPath = + if (!hasBasePve) Paths.get("amber", "requirements.txt") + else Paths.get("/tmp", "requirements.txt") + + val operatorRequirementsPath = Review Comment: `requirementsPath`/`operatorRequirementsPath` are resolved via relative paths (`Paths.get("amber", ...)`), which depends on the process working directory. In packaged deployments this can easily break PVE creation. Consider making these paths configurable or resolving them from a known install location/resources. ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> Review Comment: The delete button for installed user packages has no click handler, so it currently can’t toggle/remove a package (even though `deleteToggle` exists). Add a handler (or remove the button until delete is implemented). ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper Review Comment: Unused import: `com.fasterxml.jackson.databind.ObjectMapper` isn’t referenced. With the repo’s unused-import cleanup (scalafix/remove-unused), this should be removed to keep CI passing. ```suggestion ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): ChunkedOutput[String] = { + val queue = new LinkedBlockingQueue[String]() + val chunkedOutput = new ChunkedOutput[String](classOf[String]) + + Future { + try { + + if (!PveManager.pveExists(cuid, pveName)) { + PveManager.createNewPve(cuid, queue, pveName) + } + + } catch { + case e: Exception => + queue.put(s"[ERR] ${e.getMessage}") + } finally { + queue.put("__DONE__") + } + } + + Future { + var done = false + while (!done) { + val line = queue.take() + if (line == "__DONE__") { + chunkedOutput.write("data: __DONE__\n\n") + done = true + } else chunkedOutput.write(s"data: $line\n\n") + } + chunkedOutput.close() + } + + chunkedOutput + } + + // -------------------------------------------------- + // Get installed packages + // -------------------------------------------------- + @GET + @Path("/packages") + @Produces(Array(MediaType.APPLICATION_JSON)) + def getInstalledPackages( + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): util.Map[String, util.List[String]] = { + try { + + println(s"[PVE] HIT getInstalledPackages cuid=$cuid") + + val (systemPkgsRaw, userPkgsRaw) = PveManager.getSystemAndUserPackages(cuid, pveName) + + println(s"[PVE] raw systemPkgsRaw isNull=${systemPkgsRaw == null} value=$systemPkgsRaw") + println(s"[PVE] raw userPkgsRaw isNull=${userPkgsRaw == null} value=$userPkgsRaw") + + val systemPkgs = Option(systemPkgsRaw).getOrElse(Seq.empty[String]).toList.asJava + val userPkgs = Option(userPkgsRaw).getOrElse(Seq.empty[String]).toList.asJava + + val resp = Map("system" -> systemPkgs, "user" -> userPkgs).asJava + + println( + s"[PVE] returning keys=${resp.keySet()} systemSize=${systemPkgs.size()} userSize=${userPkgs.size()}" + ) + + resp + + } catch { + case e: Exception => + e.printStackTrace() + Map( + "system" -> List(s"Error: ${e.getMessage}").asJava, + "user" -> List.empty[String].asJava + ).asJava Review Comment: On exception, this endpoint returns HTTP 200 with an "Error: ..." string inside the `system` packages list. This makes client-side error handling brittle and may leak internal details. Prefer returning a non-2xx status (e.g., `InternalServerErrorException`) with a clear message payload. ```suggestion throw new InternalServerErrorException("Failed to get installed packages.") ``` ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts: ########## @@ -637,4 +670,257 @@ export class ComputingUnitSelectionComponent implements OnInit { this.computingUnitStatusService.refreshComputingUnitList(); } } + + private makeEmptyPve(expanded: boolean): PveDraft { + return { + id: this.nextPveId++, + name: "", + userPackages: [], + newPackages: [{ name: "", operator: "==", version: "" }], + deletingPackages: [], + pipOutput: "", + prettyPipOutput: "", + expanded, + isInstalling: false, + }; + } + + addEnvironment(): void { + this.pves.push(this.makeEmptyPve(true)); + } + + trackByPveId(_index: number, pve: PveDraft): number { + return pve.id; + } + + showPVEmodalVisible(): void { Review Comment: Method name `showPVEmodalVisible()` is confusing/grammatically off (it reads like a boolean). Consider renaming to something action-oriented like `openPveModal()`/`showPveModal()` to match what it does (sets visibility + loads PVEs). ```suggestion showPveModal(): void { ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import java.io.{File, RandomAccessFile} +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths, StandardOpenOption} +import java.util.concurrent.BlockingQueue +import scala.collection.mutable.Map +import scala.jdk.CollectionConverters._ +import scala.sys.process._ + +/** + * PveManager is responsible for managing Python Virtual Environments (PVEs) + * for each Computing Unit + * + * It supports: + * - Creating and initializing isolated Python environments + * - Installing and uninstalling Python packages + * - Tracking system vs user-installed packages via metadata files + * - Streaming pip output logs back to the caller + * + * Each PVE is stored under: + * /tmp/texera-pve/venvs/{cuid}/{pveName}/ + * + * The structure includes: + * - pve/ -> actual virtual environment + * - metadata/ -> package tracking (system + user) + */ + +object PveManager { + + private val VenvRoot: Path = Paths.get("/tmp/texera-pve/venvs") + + private def cuidDir(cuid: Int, pvename: String): Path = { + val dir = VenvRoot.resolve(cuid.toString).resolve(pvename) + Files.createDirectories(dir) + dir + } + + private def pveDir(cuid: Int, pveName: String): Path = + cuidDir(cuid, pveName).resolve("pve") + Review Comment: `cuidDir()` unconditionally calls `Files.createDirectories(dir)`, and it’s used by simple path helpers like `pythonBinPath`/`pipBinPath`. This means read-only operations such as `pveExists()` will create on-disk directories as a side effect (and user input controls the path). Consider separating “compute path” from “ensure directory exists” so existence checks don’t mutate the filesystem. ```suggestion private def cuidDir(cuid: Int, pvename: String): Path = VenvRoot.resolve(cuid.toString).resolve(pvename) private def ensureDir(path: Path): Path = { Files.createDirectories(path) path } private def pveDir(cuid: Int, pveName: String): Path = cuidDir(cuid, pveName).resolve("pve") private def ensurePveDir(cuid: Int, pveName: String): Path = ensureDir(pveDir(cuid, pveName)) ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): ChunkedOutput[String] = { + val queue = new LinkedBlockingQueue[String]() + val chunkedOutput = new ChunkedOutput[String](classOf[String]) + + Future { + try { + + if (!PveManager.pveExists(cuid, pveName)) { + PveManager.createNewPve(cuid, queue, pveName) + } + + } catch { + case e: Exception => + queue.put(s"[ERR] ${e.getMessage}") + } finally { + queue.put("__DONE__") + } + } + + Future { + var done = false + while (!done) { + val line = queue.take() + if (line == "__DONE__") { + chunkedOutput.write("data: __DONE__\n\n") + done = true + } else chunkedOutput.write(s"data: $line\n\n") + } + chunkedOutput.close() + } + + chunkedOutput + } + + // -------------------------------------------------- + // Get installed packages + // -------------------------------------------------- + @GET + @Path("/packages") + @Produces(Array(MediaType.APPLICATION_JSON)) + def getInstalledPackages( + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): util.Map[String, util.List[String]] = { + try { + + println(s"[PVE] HIT getInstalledPackages cuid=$cuid") + + val (systemPkgsRaw, userPkgsRaw) = PveManager.getSystemAndUserPackages(cuid, pveName) + + println(s"[PVE] raw systemPkgsRaw isNull=${systemPkgsRaw == null} value=$systemPkgsRaw") Review Comment: `getInstalledPackages` is using `println` for server-side logging. This bypasses the service logger configuration and can spam stdout in production. Switch to the project logger (SLF4J / LazyLogging) with an appropriate log level. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) Review Comment: `PackageResponse` is declared but never used. With remove-unused tooling enabled, this should either be removed or used as the response type to avoid CI lint failures. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import java.io.{File, RandomAccessFile} +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths, StandardOpenOption} +import java.util.concurrent.BlockingQueue +import scala.collection.mutable.Map +import scala.jdk.CollectionConverters._ +import scala.sys.process._ + +/** + * PveManager is responsible for managing Python Virtual Environments (PVEs) + * for each Computing Unit + * + * It supports: + * - Creating and initializing isolated Python environments + * - Installing and uninstalling Python packages + * - Tracking system vs user-installed packages via metadata files + * - Streaming pip output logs back to the caller + * + * Each PVE is stored under: + * /tmp/texera-pve/venvs/{cuid}/{pveName}/ + * + * The structure includes: + * - pve/ -> actual virtual environment + * - metadata/ -> package tracking (system + user) + */ + +object PveManager { + + private val VenvRoot: Path = Paths.get("/tmp/texera-pve/venvs") + + private def cuidDir(cuid: Int, pvename: String): Path = { + val dir = VenvRoot.resolve(cuid.toString).resolve(pvename) + Files.createDirectories(dir) + dir + } + + private def pveDir(cuid: Int, pveName: String): Path = + cuidDir(cuid, pveName).resolve("pve") + + private def pythonBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("python") + + private def pipBinPath(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("bin").resolve("pip") + + private def metadataDir(cuid: Int, pveName: String): Path = + pveDir(cuid, pveName).resolve("metadata") + + private def systemPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("system-packages.txt") + + private def userPackagesPath(cuid: Int, pveName: String): Path = + metadataDir(cuid, pveName).resolve("user-packages.txt") + + private def writeMetadata(path: Path, lines: Seq[String]): Unit = { + if (path.getParent != null) { + Files.createDirectories(path.getParent) + } + Files.write( + path, + lines.asJava, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + StandardOpenOption.WRITE + ) + } + + private def readMetadataList(path: Path): List[String] = { + if (!Files.exists(path)) return Nil + Files.readAllLines(path).asScala.map(_.trim).filter(_.nonEmpty).toList + } + + private def pipEnv: Map[String, String] = + Map( + "PYTHONUNBUFFERED" -> "1", + "PIP_PROGRESS_BAR" -> "off", + "PIP_DISABLE_PIP_VERSION_CHECK" -> "1", + "PIP_NO_INPUT" -> "1" + ) + + def getSystemAndUserPackages(cuid: Int, pveName: String): (Seq[String], Seq[String]) = { + val sys = readMetadataList(systemPackagesPath(cuid, pveName)) + val usr = readMetadataList(userPackagesPath(cuid, pveName)) + (sys, usr) + } + + /** + * Creates a new PVE for a CU. + * + * Behavior: + * - If a base PVE exists (PVE_BASE), it clones it for faster setup + * - Otherwise, creates a fresh venv and installs dependencies + * + * Steps: + * 1. Remove existing environment (if present) + * 2. Create or copy base environment + * 3. Install system + operator dependencies + * 4. Record installed packages as system metadata + * + * Logs progress to the provided queue. + */ + def createNewPve(cuid: Int, queue: BlockingQueue[String], pveName: String): Unit = { + queue.put(s"[PVE] Creating new PVE for cuid=$cuid with name=$pveName") + + val venvDirPath = pveDir(cuid, pveName).toAbsolutePath + val python = pythonBinPath(cuid, pveName).toAbsolutePath.toString + val envVars = pipEnv + + val pveBase = sys.env.getOrElse("PVE_BASE", "/opt/pve-base") + val basePython = Paths.get(pveBase).resolve("bin").resolve("python") + val hasBasePve = Files.exists(basePython) + + val requirementsPath = + if (!hasBasePve) Paths.get("amber", "requirements.txt") + else Paths.get("/tmp", "requirements.txt") + + val operatorRequirementsPath = + if (!hasBasePve) Paths.get("amber", "operator-requirements.txt") + else Paths.get("/tmp", "operator-requirements.txt") + + if (!hasBasePve) { + if (Files.exists(venvDirPath)) { + val rmCode = Process(Seq("bash", "-lc", s"rm -rf '${venvDirPath.toString}'")).!( + ProcessLogger( + out => queue.put(s"[pve] $out"), + err => queue.put(s"[pve][ERR] $err") + ) + ) + queue.put(s"[pve] removed existing venv with exit code $rmCode") + } + + Files.createDirectories(venvDirPath.getParent) + queue.put(s"[PVE] Creating fresh local venv at ${venvDirPath.toString}") + + val createCode = Process(Seq("python3", "-m", "venv", venvDirPath.toString)).!( + ProcessLogger( + out => queue.put(s"[pve] $out"), + err => queue.put(s"[pve][ERR] $err") + ) + ) + queue.put(s"[pve] local venv creation finished with exit code $createCode") + + if (createCode != 0) { + queue.put(s"[PVE][ERR] Failed to create local venv (exit=$createCode)") + 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 + } + + Files.createDirectories(metadataDir(cuid, pveName)) + + queue.put(s"[PVE] Installing local base requirements from ${requirementsPath.toAbsolutePath}") + + val installReqCode = Process( + Seq(python, "-m", "pip", "install", "-r", requirementsPath.toString), + None, + envVars.toSeq: _* + ).!( + ProcessLogger( + out => queue.put(s"[pip] $out"), + err => queue.put(s"[pip][ERR] $err") + ) + ) + queue.put(s"[PVE] requirements install finished with exit code $installReqCode") + + if (installReqCode != 0) { + queue.put(s"[PVE][ERR] Failed to install requirements.txt (exit=$installReqCode)") + return + } + + queue.put( + s"[PVE] Installing local operator requirements from ${operatorRequirementsPath.toAbsolutePath}" + ) + + val installOperatorReqCode = Process( + Seq(python, "-m", "pip", "install", "-r", operatorRequirementsPath.toString), + None, + envVars.toSeq: _* + ).!( + ProcessLogger( + out => queue.put(s"[pip] $out"), + err => queue.put(s"[pip][ERR] $err") + ) + ) + queue.put( + s"[PVE] operator requirements install finished with exit code $installOperatorReqCode" + ) + + if (installOperatorReqCode != 0) { + queue.put( + s"[PVE][ERR] Failed to install operator-requirements.txt (exit=$installOperatorReqCode)" + ) + return + } + + queue.put("[PVE] Running pip freeze") + val freezeOutput = Process(Seq(python, "-m", "pip", "freeze"), None, envVars.toSeq: _*).!! + val installedLines = freezeOutput.split("\n").map(_.trim).filter(_.nonEmpty).toSeq + + writeMetadata(systemPackagesPath(cuid, pveName), installedLines) + writeMetadata(userPackagesPath(cuid, pveName), Seq.empty) + + queue.put(s"[PVE] Created new local environment for cuid=$cuid") + return + } + + if (Files.exists(venvDirPath)) { + val rmCode = Process(Seq("bash", "-lc", s"rm -rf '${venvDirPath.toString}'")).!( + ProcessLogger( + out => queue.put(s"[pve] $out"), + err => queue.put(s"[pve][ERR] $err") + ) + ) + queue.put(s"[pve] removed existing venv with exit code $rmCode") + } + + Files.createDirectories(venvDirPath.getParent) + queue.put(s"[PVE] Copying base venv from $pveBase to ${venvDirPath.toString}") + + val copyCode = Process(Seq("bash", "-lc", s"cp -a '${pveBase}' '${venvDirPath.toString}'")).!( + ProcessLogger( + out => queue.put(s"[pve] $out"), + err => queue.put(s"[pve][ERR] $err") + ) Review Comment: The base venv copy runs via `bash -lc` with interpolated paths (`PVE_BASE` env var + computed destination). Prefer invoking commands without a shell (or use NIO copy) so paths are passed as arguments and can’t be interpreted by the shell. ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <!-- NEW PACKAGES --> + <div + *ngFor="let pkg of pve.newPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + placeholder="Package Name" /> + </div> + + <div class="field operator operator-select"> + <label style="visibility: hidden">Op</label> + <nz-select nzPlaceHolder="Select"> + <nz-option + nzValue="==" + nzLabel="=="></nz-option> + <nz-option + nzValue=">=" + nzLabel=">="></nz-option> + <nz-option + nzValue="<=" + nzLabel="<="></nz-option> + </nz-select> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + placeholder="Package Version (optional)" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <div class="add-btn"> + <button + nz-button + nzType="primary" + nzShape="circle"> Review Comment: The “+” button under the package list has no `(click)` handler, so users can’t add additional package rows into `pve.newPackages`. Add a handler that pushes a new empty `PackageRow`. ```suggestion nzShape="circle" (click)="pve.newPackages.push({ deleteToggle: false })"> ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): ChunkedOutput[String] = { + val queue = new LinkedBlockingQueue[String]() + val chunkedOutput = new ChunkedOutput[String](classOf[String]) + + Future { + try { + + if (!PveManager.pveExists(cuid, pveName)) { + PveManager.createNewPve(cuid, queue, pveName) + } + + } catch { + case e: Exception => + queue.put(s"[ERR] ${e.getMessage}") + } finally { + queue.put("__DONE__") + } + } + + Future { + var done = false + while (!done) { + val line = queue.take() + if (line == "__DONE__") { + chunkedOutput.write("data: __DONE__\n\n") + done = true + } else chunkedOutput.write(s"data: $line\n\n") + } + chunkedOutput.close() + } + + chunkedOutput + } + + // -------------------------------------------------- + // Get installed packages + // -------------------------------------------------- + @GET + @Path("/packages") + @Produces(Array(MediaType.APPLICATION_JSON)) + def getInstalledPackages( + @QueryParam("cuid") cuid: Int, + @QueryParam("pveName") pveName: String + ): util.Map[String, util.List[String]] = { + try { + + println(s"[PVE] HIT getInstalledPackages cuid=$cuid") + + val (systemPkgsRaw, userPkgsRaw) = PveManager.getSystemAndUserPackages(cuid, pveName) + + println(s"[PVE] raw systemPkgsRaw isNull=${systemPkgsRaw == null} value=$systemPkgsRaw") + println(s"[PVE] raw userPkgsRaw isNull=${userPkgsRaw == null} value=$userPkgsRaw") + + val systemPkgs = Option(systemPkgsRaw).getOrElse(Seq.empty[String]).toList.asJava + val userPkgs = Option(userPkgsRaw).getOrElse(Seq.empty[String]).toList.asJava + + val resp = Map("system" -> systemPkgs, "user" -> userPkgs).asJava + + println( + s"[PVE] returning keys=${resp.keySet()} systemSize=${systemPkgs.size()} userSize=${userPkgs.size()}" + ) + + resp + + } catch { + case e: Exception => + e.printStackTrace() + Map( + "system" -> List(s"Error: ${e.getMessage}").asJava, + "user" -> List.empty[String].asJava + ).asJava + } + } + + // -------------------------------------------------- + // Fetch PVEs + // -------------------------------------------------- + @GET + @Path("/pves") + @Produces(Array(MediaType.APPLICATION_JSON)) + def fetchPVEs(@QueryParam("cuid") cuid: Int): util.List[util.Map[String, Object]] = { + try { + println(s"[PVE] HIT getInstalledPackages cuid=$cuid") Review Comment: Log message in `fetchPVEs` says "HIT getInstalledPackages" (copy/paste), which will be misleading during debugging. Update it to reflect the actual endpoint (fetch PVEs). ```suggestion println(s"[PVE] HIT fetchPVEs cuid=$cuid") ``` ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts: ########## @@ -637,4 +670,257 @@ export class ComputingUnitSelectionComponent implements OnInit { this.computingUnitStatusService.refreshComputingUnitList(); } } + + private makeEmptyPve(expanded: boolean): PveDraft { + return { + id: this.nextPveId++, + name: "", + userPackages: [], + newPackages: [{ name: "", operator: "==", version: "" }], + deletingPackages: [], + pipOutput: "", + prettyPipOutput: "", + expanded, + isInstalling: false, + }; + } + + addEnvironment(): void { + this.pves.push(this.makeEmptyPve(true)); + } + + trackByPveId(_index: number, pve: PveDraft): number { + return pve.id; + } + + showPVEmodalVisible(): void { + this.pveModalVisible = true; + this.getPVEs(); + } + + getPVEs(): void { + const cuId: number | undefined = this.selectedComputingUnit?.computingUnit.cuid; + + if (cuId == null) { + this.notificationService.error("No computing unit selected. Please select a CU first."); + return; + } + + this.workflowPveService.setCuid(cuId); + + this.workflowPveService + .fetchPVEs(cuId) + .pipe(untilDestroyed(this)) + .subscribe({ + next: (resp: PvePackageResponse[]) => { + this.pves = resp.map(pve => ({ + id: this.nextPveId++, + name: pve.pveName, + expanded: false, + isInstalling: false, + pipOutput: "", + prettyPipOutput: "", + userPackages: (pve.userPackages ?? []).map(pkg => { + const [name, version] = pkg.split("=="); + return { + name: name.trim(), + version: (version ?? "").trim(), + deleteToggle: false, + }; + }), + newPackages: [], + deletingPackages: [], + })); + + if (resp.length > 0) { + this.workflowPveService.setPveName(resp[0].pveName); + + this.workflowPveService + .getInstalledPackages() + .pipe(untilDestroyed(this)) + .subscribe({ + next: installedResp => { + this.systemPackages = installedResp.system.map(pkgStr => { + const [name, version] = pkgStr.split("=="); + return { + name: name.trim(), + version: (version ?? "").trim(), + }; + }); + }, + error: (err: unknown) => { + console.error("Failed to fetch system packages:", err); + this.systemPackages = []; + }, + }); + } else { + this.systemPackages = []; + } + }, + error: (err: unknown) => { + console.error("Failed to fetch PVEs:", err); + this.pves = []; + this.systemPackages = []; + }, + }); + } + + scrollToBottomOfPipModal(index: number) { + setTimeout(() => { + const pre = document.getElementById(`pip-log-${index}`) as HTMLElement | null; + if (pre) { + pre.scrollTop = pre.scrollHeight; + } + }, 50); + } + + // Converts raw pip output for UI rendering by escaping unsafe characters and + // applying styling to exit codes, errors, warnings, and common success messages. + updatePrettyPipOutput(index: number) { + const env = this.pves[index]; + + const escapeHtml = (s: string) => + s + .replace(/&/g, "&") + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); + + const raw = env.pipOutput ?? ""; + const safe = escapeHtml(raw); + + env.prettyPipOutput = safe + .replace(/^(\[pip\].*finished with exit code\s+0.*)$/gm, "<span class=\"pip-exit ok\"><strong>$1</strong></span>") + .replace(/^(\[pip\].*finished with exit code\s+1.*)$/gm, "<span class=\"pip-exit err\"><strong>$1</strong></span>") + .replace( + /^(\[pip\].*finished with exit code\s+([2-9]\d*).*)$/gm, + "<span class=\"pip-exit err\"><strong>$1</strong></span>" + ) + .replace(/ERROR/g, "<span class=\"error\">ERROR</span>") + .replace(/WARNING/g, "<span class=\"warning\">WARNING</span>") + .replace(/already satisfied/g, "<span class=\"success\">already satisfied</span>") + .replace(/\n/g, "<br/>"); + } + + createVirtualEnvironment(index: number): void { + const cuId = this.selectedComputingUnit?.computingUnit.cuid; + const env = this.pves[index]; + + if (cuId == null) { + this.notificationService.error("No computing unit selected. Please select a CU first."); + return; + } + + if (!env.name?.trim()) { + this.notificationService.error("Environment name cannot be empty."); + return; + } + + const trimmedName = env.name.trim().toLowerCase(); + + if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) { + this.notificationService.error("Environment name must contain only letters and numbers."); + return; + } + + const duplicateExists = this.pves.some( + (pve, i) => i !== index && (pve.name ?? "").trim().toLowerCase() === trimmedName + ); + + if (duplicateExists) { + this.notificationService.error("An environment with this name already exists."); + return; + } + + env.isInstalling = true; + + this.workflowPveService.setCuid(cuId); + this.workflowPveService.setPveName(env.name); + + const packageArray: string[] = []; + + for (const p of env.newPackages) { + const name = (p.name ?? "").trim(); + const version = (p.version ?? "").trim(); + const op = (p.operator ?? "==").trim() || "=="; + + if (name !== "") { + const formatted = version !== "" ? `${name}${op}${version}` : name; + packageArray.push(formatted); + } + } + + // if (packageArray.length === 0) { + // env.isInstalling = false; + // return; + // } + + const token = localStorage.getItem("access_token") ?? ""; + const tokenParam = token ? `&access-token=${encodeURIComponent(token)}` : ""; + const query = encodeURIComponent(JSON.stringify(packageArray)); + + env.source?.close(); + env.source = undefined; + + const url = `/pve/?packages=${query}` + `&cuid=${cuId}` + `&pveName=${encodeURIComponent(env.name)}` + tokenParam; Review Comment: The request URL uses the raw `env.name`, even though you validate/dedupe against `trimmedName`. This can still create directories with trailing spaces or different casing and bypass the duplicate check. Use the normalized value when setting `pveName` and when building the URL. ```suggestion const normalizedPveName = (env.name ?? "").trim(); env.name = normalizedPveName; env.source?.close(); env.source = undefined; const url = `/pve/?packages=${query}` + `&cuid=${cuId}` + `&pveName=${encodeURIComponent(normalizedPveName)}` + tokenParam; ``` ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -0,0 +1,154 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.pythonvirtualenvironment + +import com.fasterxml.jackson.databind.ObjectMapper +import org.glassfish.jersey.server.ChunkedOutput + +import java.util.concurrent.LinkedBlockingQueue +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.Future +import scala.jdk.CollectionConverters._ +import java.util + +case class PackageResponse(system: java.util.List[String], user: java.util.List[String]) + +@Path("/pve") +@Consumes(Array(MediaType.APPLICATION_JSON)) +class PveResource { + + // -------------------------------------------------- + // Create / Install packages (SSE) + // -------------------------------------------------- + @GET + @Produces(Array("text/event-stream")) + def createPve( + @QueryParam("packages") packagesJson: String, Review Comment: `packagesJson` is accepted as a query param but is never parsed/used, so the API contract is misleading (client can send packages but nothing happens). Either remove the parameter for now or parse it and pass the package list into `PveManager`. ```suggestion ``` ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts: ########## @@ -637,4 +670,257 @@ export class ComputingUnitSelectionComponent implements OnInit { this.computingUnitStatusService.refreshComputingUnitList(); } } + + private makeEmptyPve(expanded: boolean): PveDraft { + return { + id: this.nextPveId++, + name: "", + userPackages: [], + newPackages: [{ name: "", operator: "==", version: "" }], + deletingPackages: [], + pipOutput: "", + prettyPipOutput: "", + expanded, + isInstalling: false, + }; + } + + addEnvironment(): void { + this.pves.push(this.makeEmptyPve(true)); + } + + trackByPveId(_index: number, pve: PveDraft): number { + return pve.id; + } + + showPVEmodalVisible(): void { + this.pveModalVisible = true; + this.getPVEs(); + } + + getPVEs(): void { + const cuId: number | undefined = this.selectedComputingUnit?.computingUnit.cuid; + + if (cuId == null) { + this.notificationService.error("No computing unit selected. Please select a CU first."); + return; + } + + this.workflowPveService.setCuid(cuId); + + this.workflowPveService + .fetchPVEs(cuId) + .pipe(untilDestroyed(this)) + .subscribe({ + next: (resp: PvePackageResponse[]) => { + this.pves = resp.map(pve => ({ + id: this.nextPveId++, + name: pve.pveName, + expanded: false, + isInstalling: false, + pipOutput: "", + prettyPipOutput: "", + userPackages: (pve.userPackages ?? []).map(pkg => { + const [name, version] = pkg.split("=="); + return { + name: name.trim(), + version: (version ?? "").trim(), + deleteToggle: false, + }; + }), + newPackages: [], + deletingPackages: [], + })); + + if (resp.length > 0) { + this.workflowPveService.setPveName(resp[0].pveName); + + this.workflowPveService + .getInstalledPackages() + .pipe(untilDestroyed(this)) + .subscribe({ + next: installedResp => { + this.systemPackages = installedResp.system.map(pkgStr => { + const [name, version] = pkgStr.split("=="); + return { + name: name.trim(), + version: (version ?? "").trim(), + }; + }); + }, + error: (err: unknown) => { + console.error("Failed to fetch system packages:", err); + this.systemPackages = []; + }, + }); + } else { + this.systemPackages = []; + } + }, + error: (err: unknown) => { + console.error("Failed to fetch PVEs:", err); + this.pves = []; + this.systemPackages = []; + }, + }); + } + + scrollToBottomOfPipModal(index: number) { + setTimeout(() => { + const pre = document.getElementById(`pip-log-${index}`) as HTMLElement | null; + if (pre) { + pre.scrollTop = pre.scrollHeight; + } + }, 50); + } + + // Converts raw pip output for UI rendering by escaping unsafe characters and + // applying styling to exit codes, errors, warnings, and common success messages. + updatePrettyPipOutput(index: number) { + const env = this.pves[index]; + + const escapeHtml = (s: string) => + s + .replace(/&/g, "&") + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); + + const raw = env.pipOutput ?? ""; + const safe = escapeHtml(raw); + + env.prettyPipOutput = safe + .replace(/^(\[pip\].*finished with exit code\s+0.*)$/gm, "<span class=\"pip-exit ok\"><strong>$1</strong></span>") + .replace(/^(\[pip\].*finished with exit code\s+1.*)$/gm, "<span class=\"pip-exit err\"><strong>$1</strong></span>") + .replace( + /^(\[pip\].*finished with exit code\s+([2-9]\d*).*)$/gm, + "<span class=\"pip-exit err\"><strong>$1</strong></span>" + ) + .replace(/ERROR/g, "<span class=\"error\">ERROR</span>") + .replace(/WARNING/g, "<span class=\"warning\">WARNING</span>") + .replace(/already satisfied/g, "<span class=\"success\">already satisfied</span>") + .replace(/\n/g, "<br/>"); + } + + createVirtualEnvironment(index: number): void { + const cuId = this.selectedComputingUnit?.computingUnit.cuid; + const env = this.pves[index]; + + if (cuId == null) { + this.notificationService.error("No computing unit selected. Please select a CU first."); + return; + } + + if (!env.name?.trim()) { + this.notificationService.error("Environment name cannot be empty."); + return; + } + + const trimmedName = env.name.trim().toLowerCase(); + + if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) { + this.notificationService.error("Environment name must contain only letters and numbers."); + return; + } + + const duplicateExists = this.pves.some( + (pve, i) => i !== index && (pve.name ?? "").trim().toLowerCase() === trimmedName + ); + + if (duplicateExists) { + this.notificationService.error("An environment with this name already exists."); + return; + } + + env.isInstalling = true; + + this.workflowPveService.setCuid(cuId); + this.workflowPveService.setPveName(env.name); + + const packageArray: string[] = []; + + for (const p of env.newPackages) { + const name = (p.name ?? "").trim(); + const version = (p.version ?? "").trim(); + const op = (p.operator ?? "==").trim() || "=="; + + if (name !== "") { + const formatted = version !== "" ? `${name}${op}${version}` : name; + packageArray.push(formatted); + } + } + + // if (packageArray.length === 0) { + // env.isInstalling = false; + // return; + // } + + const token = localStorage.getItem("access_token") ?? ""; + const tokenParam = token ? `&access-token=${encodeURIComponent(token)}` : ""; + const query = encodeURIComponent(JSON.stringify(packageArray)); + + env.source?.close(); + env.source = undefined; + + const url = `/pve/?packages=${query}` + `&cuid=${cuId}` + `&pveName=${encodeURIComponent(env.name)}` + tokenParam; + + const source = new EventSource(url); + env.source = source; + Review Comment: `EventSource` is created here, but there’s no guaranteed teardown on modal close or component destroy. Add centralized cleanup (close all active `env.source`) and invoke it from modal close + `ngOnDestroy` to avoid leaked connections and background updates. ########## frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html: ########## @@ -427,3 +435,220 @@ </div> </div> </ng-template> + +<!-- Modal for adding packages --> +<nz-modal + nzWrapClassName="pve-modal" + nzClassName="pve-modal" + [nzVisible]="pveModalVisible" + nzTitle="Python Environments" + (nzOnCancel)="pveModalVisible = false" + [nzFooter]="customFooter"> + <ng-template #customFooter> + <div class="footer-all"> + <button + nz-button + nzType="default" + (click)="pveModalVisible = false"> + Close + </button> + <button + nz-button + nzType="primary" + (click)="addEnvironment()"> + Add Environment + </button> + </div> + </ng-template> + + <ng-container *nzModalContent> + <!-- Shared system packages --> + <div class="system-section"> + <nz-collapse> + <nz-collapse-panel nzHeader="System Packages (read-only)"> + <div class="system-panel-inner"> + <div + *ngFor="let pkg of systemPackages" + class="package-row system-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.name" /> + </div> + <div class="field"> + <label>Version</label> + <input + nz-input + class="system-input" + [disabled]="true" + [ngModel]="pkg.version" /> + </div> + </div> + </div> + </div> + </nz-collapse-panel> + </nz-collapse> + </div> + + <!-- Environments --> + <nz-collapse> + <nz-collapse-panel + *ngFor="let pve of pves; let envIndex = index; trackBy: trackByPveId" + [nzActive]="pve.expanded" + (nzActiveChange)="pve.expanded = $event" + [nzHeader]="headerTpl"> + <!-- Custom header --> + <ng-template #headerTpl> + <div class="env-header"> + <span class="env-title"> {{ pve.name }} </span> + <span + class="env-actions" + (click)="$event.stopPropagation()"> + </span> + </div> + </ng-template> + + <!-- Panel body --> + <div class="ve-form"> + <div class="fieldRow"> + <label class="fieldLabel">Virtual Environment Name</label> + <input + nz-input + class="fieldInput" + placeholder="Environment Name" + [(ngModel)]="pve.name" /> + </div> + + <!-- USER PACKAGES --> + <div + class="section-title" + *ngIf="pve.userPackages.length > 0"> + Installed User Packages + </div> + + <div + *ngFor="let pkg of pve.userPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + [ngModel]="pkg.name" + [disabled]="true" /> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input + [ngModel]="pkg.version" + [disabled]="true" /> + </div> + </div> + + <button + nz-button + nzType="default" + nzShape="circle" + nzDanger + [ngClass]="{ 'highlighted-btn': pkg.deleteToggle }"> + <i + nz-icon + nzType="delete"></i> + </button> + </div> + + <!-- NEW PACKAGES --> + <div + *ngFor="let pkg of pve.newPackages; let i = index" + class="package-row"> + <div class="package-inputs"> + <div class="field"> + <label>Package</label> + <input + nz-input + placeholder="Package Name" /> + </div> + + <div class="field operator operator-select"> + <label style="visibility: hidden">Op</label> + <nz-select nzPlaceHolder="Select"> + <nz-option + nzValue="==" + nzLabel="=="></nz-option> + <nz-option + nzValue=">=" + nzLabel=">="></nz-option> + <nz-option + nzValue="<=" + nzLabel="<="></nz-option> + </nz-select> + </div> + + <div class="field"> + <label>Version</label> + <input + nz-input Review Comment: The “New Packages” version input is missing `[(ngModel)]` binding to `pkg.version`, so version constraints entered in the UI won’t be included in the request. ```suggestion nz-input [(ngModel)]="pkg.version" ``` -- 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]
