You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "SarahAsad23 (via GitHub)" <gi...@apache.org> on 2026/06/09 00:12:29 UTC
[PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
SarahAsad23 opened a new pull request, #5577:
URL: https://github.com/apache/texera/pull/5577
<!--
Thanks for sending a pull request (PR)! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
[Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md)
2. Ensure you have added or run the appropriate tests for your PR
3. If the PR is work in progress, mark it a draft on GitHub.
4. Please write your PR title to summarize what this PR proposes, we
are following Conventional Commits style for PR titles as well.
5. Be sure to keep the PR description updated to reflect all changes.
-->
### What changes were proposed in this PR?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes. Here are some tips for you:
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
3. If it is a refactoring, clarify what has been changed.
3. It would be helpful to include a before-and-after comparison using
screenshots or GIFs.
4. Please consider writing useful notes for better and faster reviews.
-->
### Any related issues, documentation, discussions?
<!--
Please use this section to link other resources if not mentioned already.
1. If this PR fixes an issue, please include `Fixes #1234`, `Resolves #1234`
or `Closes #1234`. If it is only related, simply mention the issue number.
2. If there is design documentation, please add the link.
3. If there is a discussion in the mailing list, please add the link.
-->
### How was this PR tested?
<!--
If tests were added, say they were added here. Or simply mention that if the PR
is tested with existing test cases. Make sure to include/update test cases that
check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how
you tested step by step, ideally copy and paste-able, so that other reviewers can
test and check, and descendants can verify in the future. If tests were not added,
please describe why they were not added and/or why it was difficult to add.
-->
### Was this PR authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this PR,
please include the phrase: 'Generated-by: ' followed by the name of the tool
and its version. If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5577:
URL: https://github.com/apache/texera/pull/5577#issuecomment-4688629218
<!-- texera-benchmarks-comment -->
## Arrow Flight E2E bench
| batch | schema_w | str_len | n_batches | tuples/s | MB/s | p50 ms | p99 ms | total ms |
|---:|---:|---:|---:|---:|---:|---:|---:|---:|
| 10 | 10 | 64 | 20 | 361 | 0.220 | 27.10 | 34.53 | 554.34 |
| 100 | 10 | 64 | 20 | 755 | 0.461 | 130.89 | 169.63 | 2650.20 |
| 1000 | 10 | 64 | 20 | 905 | 0.552 | 1104.01 | 1133.99 | 22095.91 |
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,554.34,200,128000,361,0.220,27100.20,34534.75,34534.75
1,100,10,64,20,2650.20,2000,1280000,755,0.461,130886.35,169625.16,169625.16
2,1000,10,64,20,22095.91,20000,12800000,905,0.552,1104014.30,1133986.70,1133986.70
```
</details>
[Full workflow run](https://github.com/apache/texera/actions/runs/27401292223)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 commented on code in PR #5577:
URL: https://github.com/apache/texera/pull/5577#discussion_r3391715679
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -47,10 +51,15 @@ object PveManager {
userPackages: Seq[String]
)
+ case class StoredPve(veid: Int, name: String, packagesJson: String)
+
private val VenvRoot: Path = Paths.get("/tmp/texera-pve/venvs")
private val SafePveName = "^[A-Za-z0-9._-]+$".r
+ def isValidPveName(name: String): Boolean =
+ name != null && SafePveName.pattern.matcher(name).matches()
Review Comment:
The database column is `VARCHAR(128)` but `isValidPveName` only checks the charset. Add a length guard here (e.g. `name.length <= 128`).
##########
frontend/src/app/dashboard/component/user/user-venv/user-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">Environments</h2>
+ <div class="button-group">
+ <button
+ nz-button
+ class="create-btn"
+ (click)="showPveModal()"
+ title="Create Environment">
+ <i
+ nz-icon
+ nzType="file-add"
+ nzTheme="outline"></i>
+ <span>Create Environment</span>
+ </button>
+ </div>
+ </nz-card>
+
+ <nz-card
+ class="section-list-container"
+ [nzBodyStyle]="{ height: '100%' }">
+ <div class="python-env-page">
+ <div
+ *ngIf="pves.length === 0"
+ class="python-env-page-empty">
+ No environments yet. Click <strong>Create Python Venv</strong> to create one.
+ </div>
+
+ <ul
+ *ngIf="pves.length > 0"
+ class="python-env-page-list">
+ <li
+ *ngFor="let pve of pves; let i = index; trackBy: trackByVeid"
+ class="python-env-page-item"
+ (click)="openExistingPve(i)">
+ <span class="python-env-name">{{ pve.name || "(unnamed)" }}</span>
+ <i
+ nz-icon
+ nzType="delete"
+ class="python-env-delete-icon"
+ nz-tooltip
+ nzTooltipTitle="Delete environment"
+ (click)="confirmDeletePve(i); $event.stopPropagation()"
+ role="button"
+ aria-label="Delete Python environment">
+ </i>
+ </li>
+ </ul>
+ </div>
+ </nz-card>
+</div>
+
+<nz-modal
+ nzWrapClassName="pve-modal"
+ nzClassName="pve-modal"
+ [nzVisible]="pveModalVisible"
+ nzTitle="Python Environments"
Review Comment:
Ditto.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala:
##########
@@ -47,13 +62,91 @@ class PveResource {
Map("system" -> systemPkgs).asJava
} catch {
case e: Exception =>
- e.printStackTrace()
+ logger.error("Failed to get system packages", e)
throw new InternalServerErrorException(
"Failed to get system packages."
)
}
}
+ // --------------------------------------------------
+ // 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.veid, stored.name, packages)
+ }
+ .asJava
+ }
+
+ // --------------------------------------------------
+ // Update a PVE row owned by the current user
+ // --------------------------------------------------
+ @PUT
+ @Path("/db/{veid}")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def updatePve(
+ @PathParam("veid") veid: Int,
+ payload: SavePvePayload,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ val name = Option(payload.name).map(_.trim).getOrElse("")
+ if (!PveManager.isValidPveName(name)) {
+ return Response.status(Response.Status.BAD_REQUEST).entity("invalid name").build()
+ }
+ try {
+ val packagesJson = mapper.writeValueAsString(payload.packages)
+ val updated = PveManager.updatePve(veid, sessionUser.getUid.intValue(), name, packagesJson)
+ if (updated) Response.ok(Map("veid" -> veid).asJava).build()
+ else Response.status(Response.Status.NOT_FOUND).build()
+ } catch {
+ case e: Exception =>
+ logger.error("Failed to update PVE", e)
+ throw new InternalServerErrorException(s"Failed to update PVE: ${e.getMessage}")
+ }
+ }
+
+ // --------------------------------------------------
+ // Delete a PVE row owned by the current user
+ // --------------------------------------------------
+ @DELETE
+ @Path("/db/{veid}")
+ def deletePveFromDb(@PathParam("veid") veid: Int, @Auth sessionUser: SessionUser): Response = {
+ val deleted = PveManager.deletePveFromDb(veid, 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 = {
+ val name = Option(payload.name).map(_.trim).getOrElse("")
+ if (!PveManager.isValidPveName(name)) {
+ return Response.status(Response.Status.BAD_REQUEST).entity("invalid name").build()
+ }
+ try {
+ val packagesJson = mapper.writeValueAsString(payload.packages)
+ val veid = PveManager.savePve(sessionUser.getUid.intValue(), name, packagesJson)
+ Response.ok(Map("veid" -> veid).asJava).build()
+ } catch {
+ case e: Exception =>
+ logger.error("Failed to save PVE", e)
+ throw new InternalServerErrorException(s"Failed to save PVE: ${e.getMessage}")
+ }
Review Comment:
Try to return 409 error for unique constraint violations instead of 500 error. Same for `updatePve`. Also, better to return 201 instead of 200 for success.
##########
frontend/src/app/dashboard/component/user/user-venv/user-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">Environments</h2>
+ <div class="button-group">
+ <button
+ nz-button
+ class="create-btn"
+ (click)="showPveModal()"
+ title="Create Environment">
+ <i
+ nz-icon
+ nzType="file-add"
+ nzTheme="outline"></i>
+ <span>Create Environment</span>
+ </button>
+ </div>
+ </nz-card>
+
+ <nz-card
+ class="section-list-container"
+ [nzBodyStyle]="{ height: '100%' }">
+ <div class="python-env-page">
+ <div
+ *ngIf="pves.length === 0"
+ class="python-env-page-empty">
+ No environments yet. Click <strong>Create Python Venv</strong> to create one.
Review Comment:
This says **“Create Python Venv”**, but the actual button (and the page title) is **“Create Environment”** / **“Environments”**. Suggest aligning the names.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "SarahAsad23 (via GitHub)" <gi...@apache.org>.
SarahAsad23 commented on code in PR #5577:
URL: https://github.com/apache/texera/pull/5577#discussion_r3399358724
##########
frontend/src/app/dashboard/component/user/user-venv/user-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">Environments</h2>
+ <div class="button-group">
+ <button
+ nz-button
+ class="create-btn"
+ (click)="showPveModal()"
+ title="Create Environment">
+ <i
+ nz-icon
+ nzType="file-add"
+ nzTheme="outline"></i>
+ <span>Create Environment</span>
+ </button>
+ </div>
+ </nz-card>
+
+ <nz-card
+ class="section-list-container"
+ [nzBodyStyle]="{ height: '100%' }">
+ <div class="python-env-page">
+ <div
+ *ngIf="pves.length === 0"
+ class="python-env-page-empty">
+ No environments yet. Click <strong>Create Python Venv</strong> to create one.
+ </div>
+
+ <ul
+ *ngIf="pves.length > 0"
+ class="python-env-page-list">
+ <li
+ *ngFor="let pve of pves; let i = index; trackBy: trackByVeid"
+ class="python-env-page-item"
+ (click)="openExistingPve(i)">
+ <span class="python-env-name">{{ pve.name || "(unnamed)" }}</span>
+ <i
+ nz-icon
+ nzType="delete"
+ class="python-env-delete-icon"
+ nz-tooltip
+ nzTooltipTitle="Delete environment"
+ (click)="confirmDeletePve(i); $event.stopPropagation()"
+ role="button"
+ aria-label="Delete Python environment">
+ </i>
+ </li>
+ </ul>
+ </div>
+ </nz-card>
+</div>
+
+<nz-modal
+ nzWrapClassName="pve-modal"
+ nzClassName="pve-modal"
+ [nzVisible]="pveModalVisible"
+ nzTitle="Python Environments"
Review Comment:
The modal itself is specific to Python virtual environments, so I think the UI should clearly indicate that the environment being created is a Python virtual environment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 commented on code in PR #5577:
URL: https://github.com/apache/texera/pull/5577#discussion_r3405742006
##########
sql/changelog.xml:
##########
@@ -29,6 +29,10 @@
<sqlFile path="sql/updates/23.sql"/>
</changeSet>
+ <changeSet id="24" author="sarahasad23">
+ <sqlFile path="sql/updates/24.sql/"/>
Review Comment:
Remove the trailing `/`.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5577:
URL: https://github.com/apache/texera/pull/5577#issuecomment-4654724687
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5577?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 52.17%. Comparing base ([`564ccdb`](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`362f850`](https://app.codecov.io/gh/apache/texera/commit/362f85057147a9b6757002fe6347d0c71db6828e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #5577 +/- ##
=========================================
Coverage 52.17% 52.17%
Complexity 2482 2482
=========================================
Files 1068 1068
Lines 41311 41311
Branches 4439 4439
=========================================
Hits 21556 21556
Misses 18490 18490
Partials 1265 1265
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.61% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.76% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.28% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `1.65% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.06% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.32% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.42% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.72% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [python](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.75% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.69% <ø> (ø)` | | Carriedforward from [564ccdb](https://app.codecov.io/gh/apache/texera/commit/564ccdbfd51089f31dcfdcbdba9f5a872a2f8256?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/5577?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Make Python Virtual Environment Persistent: Add Environments to Left Panel [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 merged PR #5577:
URL: https://github.com/apache/texera/pull/5577
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org