kunwp1 commented on code in PR #5577: URL: https://github.com/apache/texera/pull/5577#discussion_r3390062494
########## sql/updates/24.sql: ########## @@ -0,0 +1,38 @@ +/* + * 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. + */ + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +-- Adds the python_virtual_environments table, used to persist user-owned PVE +-- metadata (name + installed package versions) instead of relying on the +-- filesystem layout under /tmp/texera-pve/venvs. +CREATE TABLE IF NOT EXISTS python_virtual_environments +( + pveid SERIAL PRIMARY KEY, + uid INT NOT NULL, + name VARCHAR(128) NOT NULL, Review Comment: I can think of a scenario where the user can create duplicate venv names using two browser tabs. Better to add a unique key here. (Same for `texera_ddl.sql`) ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -54,6 +68,90 @@ class PveResource { } } + // -------------------------------------------------- + // List all PVEs for the current user from the database + // -------------------------------------------------- + @GET + @Path("/db") + @Produces(Array(MediaType.APPLICATION_JSON)) + def listPves(@Auth sessionUser: SessionUser): java.util.List[PveListItem] = { + PveManager + .listPvesForUser(sessionUser.getUid.intValue()) + .map { stored => + val packages: Map[String, String] = + try mapper.readValue(stored.packagesJson, packagesType).asScala.toMap + catch { case _: Throwable => Map.empty[String, String] } + PveListItem(stored.pveid, stored.name, packages) + } + .asJava + } + + // -------------------------------------------------- + // Update a PVE row owned by the current user + // -------------------------------------------------- + @PUT + @Path("/db/{pveid}") + @Produces(Array(MediaType.APPLICATION_JSON)) + def updatePve( + @PathParam("pveid") pveid: Int, + payload: SavePvePayload, + @Auth sessionUser: SessionUser + ): Response = { + val name = Option(payload.name).map(_.trim).getOrElse("") + if (name.isEmpty) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity("name is required") + .build() + } + try { + val packagesJson = mapper.writeValueAsString(payload.packages) + val updated = PveManager.updatePve(pveid, sessionUser.getUid.intValue(), name, packagesJson) + if (updated) Response.ok(Map("pveid" -> pveid).asJava).build() + else Response.status(Response.Status.NOT_FOUND).build() + } catch { + case e: Exception => + e.printStackTrace() + throw new InternalServerErrorException(s"Failed to update PVE: ${e.getMessage}") + } + } + + // -------------------------------------------------- + // Delete a PVE row owned by the current user + // -------------------------------------------------- + @DELETE + @Path("/db/{pveid}") + def deletePveFromDb(@PathParam("pveid") pveid: Int, @Auth sessionUser: SessionUser): Response = { + val deleted = PveManager.deletePveFromDb(pveid, sessionUser.getUid.intValue()) + if (deleted) Response.noContent().build() + else Response.status(Response.Status.NOT_FOUND).build() + } + + // -------------------------------------------------- + // Save a PVE (name + packages) to the database for the current user + // -------------------------------------------------- + @POST + @Path("/db") + @Produces(Array(MediaType.APPLICATION_JSON)) + def savePve(payload: SavePvePayload, @Auth sessionUser: SessionUser): Response = { Review Comment: `savePve` (and `updatePve` above) only reject an empty name. But in the PveManager, we have a name constraint `SafePveName = ^[A-Za-z0-9._-]+$`. Better to check the name here as well. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -29,9 +34,18 @@ import javax.ws.rs.DELETE import javax.ws.rs.PathParam import javax.ws.rs.core.Response +object PveResource { + case class SavePvePayload(name: String, packages: Map[String, String]) + case class PveListItem(pveid: Int, name: String, packages: Map[String, String]) +} + @Path("/pve") @Consumes(Array(MediaType.APPLICATION_JSON)) class PveResource { + import PveResource._ + + private val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule) Review Comment: Module registration isn't cheap and you are allocating it for each PveResource instance. Better to move this to `object PveResource` and reuse it. ########## frontend/src/app/dashboard/component/user/user-python-venv/user-python-venv.component.ts: ########## @@ -0,0 +1,243 @@ +/* + * 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 { Component, OnInit } from "@angular/core"; +import { FormsModule } from "@angular/forms"; +import { NgFor, NgIf } from "@angular/common"; +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; + +import { NzButtonComponent } from "ng-zorro-antd/button"; +import { NzCardComponent } from "ng-zorro-antd/card"; +import { NzIconDirective } from "ng-zorro-antd/icon"; +import { NzInputDirective } from "ng-zorro-antd/input"; +import { NzModalComponent, NzModalContentDirective, NzModalService } from "ng-zorro-antd/modal"; +import { NzOptionComponent, NzSelectComponent } from "ng-zorro-antd/select"; +import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; + +import { NotificationService } from "../../../../common/service/notification/notification.service"; +import { + UserPveRecord, + WorkflowPveService, +} from "../../../../workspace/service/virtual-environment/virtual-environment.service"; + +type PveUserPackageRow = { + name: string; + versionOp: "==" | ">=" | "<="; + version: string; + deleteToggle?: boolean; +}; + +type PveDraft = { + pveid?: number; + name: string; + newPackages: PveUserPackageRow[]; +}; + +@UntilDestroy() +@Component({ + selector: "texera-user-python-venv", + templateUrl: "./user-python-venv.component.html", + styleUrls: ["./user-python-venv.component.scss"], + imports: [ + NgIf, + NgFor, + FormsModule, + NzButtonComponent, + NzCardComponent, + NzIconDirective, + NzInputDirective, + NzModalComponent, + NzModalContentDirective, + NzSelectComponent, + NzOptionComponent, + NzTooltipDirective, + ], +}) +export class UserPythonVenvComponent implements OnInit { + // The user's PVEs (fetched from the DB), rendered as the page list. + pves: PveDraft[] = []; + + // The single PVE currently being edited in the modal. Null when modal is closed. + currentDraft: PveDraft | null = null; + + pveModalVisible = false; + saving = false; + + constructor( + private workflowPveService: WorkflowPveService, + private notificationService: NotificationService, + private modalService: NzModalService + ) {} + + ngOnInit(): void { + this.refreshPves(); + } + + confirmDeletePve(index: number): void { + const target = this.pves[index]; + if (!target) return; + const name = target.name || "(unnamed)"; + this.modalService.confirm({ + nzTitle: `Delete environment "${name}"?`, + nzContent: "This permanently removes the environment from the database.", + nzOkText: "Delete", + nzOkDanger: true, + nzOnOk: () => this.deletePve(index), + }); + } + + private refreshPves(): void { + this.workflowPveService + .listUserPves() + .pipe(untilDestroyed(this)) + .subscribe({ + next: records => { + this.pves = records.map(record => this.recordToDraft(record)); + }, + error: (err: unknown) => { + console.error("Failed to fetch Python environments", err); + this.notificationService.error("Failed to fetch Python environments."); + }, + }); + } + + private recordToDraft(record: UserPveRecord): PveDraft { + const newPackages: PveUserPackageRow[] = Object.entries(record.packages ?? {}).map(([name, raw]) => { + const match = raw?.match?.(/^(==|>=|<=)(.*)$/); + return { + name, + versionOp: (match ? match[1] : "==") as "==" | ">=" | "<=", + version: match ? match[2] : raw ?? "", + }; + }); + return { + pveid: record.pveid, + name: record.name, + newPackages, + }; + } + + showPveModal(): void { + this.currentDraft = { + name: "", + newPackages: [], + }; + this.pveModalVisible = true; + } + + openExistingPve(index: number): void { + const source = this.pves[index]; + if (!source) return; + this.currentDraft = { + pveid: source.pveid, + name: source.name, + newPackages: source.newPackages.map(p => ({ ...p })), + }; + this.pveModalVisible = true; + } + + closePveModal(): void { + this.pveModalVisible = false; + this.currentDraft = null; + } + + addPackage(): void { + this.currentDraft?.newPackages.push({ name: "", versionOp: "==", version: "" }); + } + + togglePackageDelete(pkg: PveUserPackageRow): void { + pkg.deleteToggle = !pkg.deleteToggle; + } + + saveEnvironment(): void { + const draft = this.currentDraft; + if (!draft) return; + + const trimmedName = draft.name.trim(); + if (!trimmedName) { + this.notificationService.error("Environment name is required."); + return; + } + + const conflict = this.pves.find(p => p.name.trim() === trimmedName && p.pveid !== draft.pveid); + if (conflict) { + this.notificationService.error(`An environment named "${trimmedName}" already exists.`); + return; + } + + const packages: Record<string, string> = {}; + for (const row of draft.newPackages) { + if (row.deleteToggle) continue; + const pkgName = row.name.trim(); + if (!pkgName) continue; + const pkgVersion = (row.version ?? "").trim(); + if (packages[pkgName] !== undefined) { + this.notificationService.error(`Duplicate package "${pkgName}".`); + return; + } + packages[pkgName] = pkgVersion ? `${row.versionOp}${pkgVersion}` : ""; + } + + this.saving = true; + const request$ = + draft.pveid === undefined + ? this.workflowPveService.savePve(trimmedName, packages) + : this.workflowPveService.updateUserPve(draft.pveid, trimmedName, packages); + + request$.pipe(untilDestroyed(this)).subscribe({ + next: () => { + this.saving = false; + this.notificationService.success(`Saved environment "${trimmedName}".`); + this.closePveModal(); + this.refreshPves(); + }, + error: (err: unknown) => { + this.saving = false; + console.error("Failed to save PVE", err); + this.notificationService.error("Failed to save Python environment."); + }, + }); + } + + trackByIndex(index: number): number { Review Comment: Tracking by index is risky because you can mess up the ordering of the list. Track with pveid instead. ########## access-control-service/src/main/scala/org/apache/texera/service/resource/AccessControlResource.scala: ########## @@ -233,6 +233,16 @@ class AccessControlResource extends LazyLogging { AccessControlResource.authorize(uriInfo, headers, Option(body).map(_.trim).filter(_.nonEmpty)) } + @PUT Review Comment: Have you tested in k8s environment? I don't think the browser can reach this new route in k8s. Up to your decision to make it work on k8s deployment or create a follow-up PR for k8s deployment. If the latter, please mention it in the PR description. ########## sql/texera_ddl.sql: ########## @@ -213,6 +214,16 @@ CREATE TABLE IF NOT EXISTS workflow_computing_unit FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE ); +-- python_virtual_environments table +CREATE TABLE IF NOT EXISTS python_virtual_environments Review Comment: Consider naming the table to "virtual_environments" if you are going to reuse this table for R virtual environments. ########## amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala: ########## @@ -54,6 +68,90 @@ class PveResource { } } + // -------------------------------------------------- + // List all PVEs for the current user from the database + // -------------------------------------------------- + @GET + @Path("/db") + @Produces(Array(MediaType.APPLICATION_JSON)) + def listPves(@Auth sessionUser: SessionUser): java.util.List[PveListItem] = { + PveManager + .listPvesForUser(sessionUser.getUid.intValue()) + .map { stored => + val packages: Map[String, String] = + try mapper.readValue(stored.packagesJson, packagesType).asScala.toMap + catch { case _: Throwable => Map.empty[String, String] } + PveListItem(stored.pveid, stored.name, packages) + } + .asJava + } + + // -------------------------------------------------- + // Update a PVE row owned by the current user + // -------------------------------------------------- + @PUT + @Path("/db/{pveid}") + @Produces(Array(MediaType.APPLICATION_JSON)) + def updatePve( + @PathParam("pveid") pveid: Int, + payload: SavePvePayload, + @Auth sessionUser: SessionUser + ): Response = { + val name = Option(payload.name).map(_.trim).getOrElse("") + if (name.isEmpty) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity("name is required") + .build() + } + try { + val packagesJson = mapper.writeValueAsString(payload.packages) + val updated = PveManager.updatePve(pveid, sessionUser.getUid.intValue(), name, packagesJson) + if (updated) Response.ok(Map("pveid" -> pveid).asJava).build() + else Response.status(Response.Status.NOT_FOUND).build() + } catch { + case e: Exception => + e.printStackTrace() Review Comment: Can you change all the `e.printStackTrace()` to `LazyLogging` in this file? We don't use `e.printStackTrace()` to print the errors. ########## frontend/src/app/dashboard/component/user/user-python-venv/user-python-venv.component.html: ########## @@ -0,0 +1,183 @@ +<!-- + 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. +--> + +<div class="section-container subsection-grid-container"> + <nz-card class="section-title"> + <h2 class="page-title">Python Venvs</h2> Review Comment: Naming is inconsistent across the feature: the sidebar says "Environments", this heading and the create button say "Python Venvs", and the modal title says "Python Environments". Choose one label and use that. -- 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]
