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/05/15 00:37:25 UTC
[PR] feat: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
SarahAsad23 opened a new pull request, #5069:
URL: https://github.com/apache/texera/pull/5069
<!--
Thanks for sending a pull request (PR)! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
[Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md)
2. Ensure you have added or run the appropriate tests for your PR
3. If the PR is work in progress, mark it a draft on GitHub.
4. Please write your PR title to summarize what this PR proposes, we
are following Conventional Commits style for PR titles as well.
5. Be sure to keep the PR description updated to reflect all changes.
-->
### What changes were proposed in this PR?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes. Here are some tips for you:
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
3. If it is a refactoring, clarify what has been changed.
3. It would be helpful to include a before-and-after comparison using
screenshots or GIFs.
4. Please consider writing useful notes for better and faster reviews.
-->
This PR is an extension of PR https://github.com/apache/texera/pull/4484, https://github.com/apache/texera/pull/4902, and #5035. This PR adds support for running Python UDFs inside user configured Python Virtual Environments. Previously, UDFs executed using the default system environment, which limited dependency isolation and reproducibility across workflows. With this change, users can associate a PVE with a UDF execution environment, allowing workflows to run with custom package installations and environment specific dependencies.
### 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.
-->
his change is part of ongoing efforts to support environment isolation and reproducibility within Texera. Related issue includes https://github.com/apache/texera/issues/4296. This PR closes sub-issue #5068
### 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: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 commented on code in PR #5069:
URL: https://github.com/apache/texera/pull/5069#discussion_r3260786595
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorker.scala:
##########
@@ -165,15 +165,47 @@ class PythonWorkflowWorker(
clientThreadExecutor.submit(pythonProxyClient)
}
+ private def choosePythonBin(): String = {
+ val fallback =
+ if (pythonENVPath.trim.isEmpty || pythonENVPath.trim == "Default") "python3"
+ else pythonENVPath
+
+ val cuidOpt = workerConfig.cuid
+ .orElse(sys.env.get("TEXERA_CUID").flatMap(s => scala.util.Try(s.toInt).toOption))
+ val pveName = workerConfig.pveName.trim
+
+ if (cuidOpt.isEmpty || pveName.isEmpty) {
+ return fallback
+ }
+
+ val candidate = Paths.get(
+ "/tmp/texera-pve/venvs",
+ cuidOpt.get.toString,
+ pveName,
Review Comment:
Suggested by Claude:
A `pveName` like `../../../usr/local/bin` (or one that resolves through a symlink) lets a workflow definition pin an arbitrary executable as the Python interpreter, as long as a `pve/bin/python` file exists at the resolved location.
UDF authors already have code-exec privileges on their own workflows, but workflow JSON is imported/shared between users and this turns workflow import into arbitrary local binary execution.
Validate before constructing the path, e.g. reject anything that doesn't match `^[A-Za-z0-9._-]+$`, and verify the resolved path is still under `VenvRoot` (`candidate.normalize().startsWith(VenvRoot)`).
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorker.scala:
##########
@@ -165,15 +165,47 @@ class PythonWorkflowWorker(
clientThreadExecutor.submit(pythonProxyClient)
}
+ private def choosePythonBin(): String = {
+ val fallback =
+ if (pythonENVPath.trim.isEmpty || pythonENVPath.trim == "Default") "python3"
+ else pythonENVPath
+
+ val cuidOpt = workerConfig.cuid
+ .orElse(sys.env.get("TEXERA_CUID").flatMap(s => scala.util.Try(s.toInt).toOption))
Review Comment:
Why the `TEXERA_CUID` env-var fallback? It introduces a new env with no other reader in the codebase. Can we just drop the fallback and if `workerConfig.cuid` is `None`, just use fallback python?
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorker.scala:
##########
@@ -165,15 +165,47 @@ class PythonWorkflowWorker(
clientThreadExecutor.submit(pythonProxyClient)
}
+ private def choosePythonBin(): String = {
+ val fallback =
Review Comment:
Better to utilize `PythonUtils.scala` for the fallback. Also, if we use such "Default" magic string, we need to prevent users for creating virtual environments with the name "Default". Can we avoid using such magic string? (e.g., add a checkbox in the property panel if the user wants to specify venv. If user clicks it, then show the drop-down)
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorker.scala:
##########
@@ -165,15 +165,47 @@ class PythonWorkflowWorker(
clientThreadExecutor.submit(pythonProxyClient)
}
+ private def choosePythonBin(): String = {
+ val fallback =
+ if (pythonENVPath.trim.isEmpty || pythonENVPath.trim == "Default") "python3"
+ else pythonENVPath
+
+ val cuidOpt = workerConfig.cuid
+ .orElse(sys.env.get("TEXERA_CUID").flatMap(s => scala.util.Try(s.toInt).toOption))
+ val pveName = workerConfig.pveName.trim
+
+ if (cuidOpt.isEmpty || pveName.isEmpty) {
+ return fallback
+ }
+
+ val candidate = Paths.get(
+ "/tmp/texera-pve/venvs",
Review Comment:
Hardcoded `/tmp/texera-pve/venvs` duplicates `PveManager.VenvRoot`. `PveManager` already has a `private def pythonBinPath(cuid, pveName)` that constructs exactly this path. Expose it (or a `getPythonBin(cuid, pveName): Option[Path]` that does the exists/executable check too) and call it from here.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorker.scala:
##########
@@ -165,15 +165,47 @@ class PythonWorkflowWorker(
clientThreadExecutor.submit(pythonProxyClient)
}
+ private def choosePythonBin(): String = {
+ val fallback =
+ if (pythonENVPath.trim.isEmpty || pythonENVPath.trim == "Default") "python3"
+ else pythonENVPath
+
+ val cuidOpt = workerConfig.cuid
+ .orElse(sys.env.get("TEXERA_CUID").flatMap(s => scala.util.Try(s.toInt).toOption))
+ val pveName = workerConfig.pveName.trim
+
+ if (cuidOpt.isEmpty || pveName.isEmpty) {
+ return fallback
Review Comment:
Remove explicit `return` and use a `if/else` if you want to use idiomatic Scala:
```scala
if (cuidOpt.isEmpty || pveName.isEmpty) {
fallback
} else {
val candidate = Paths.get("/tmp/texera-pve/venvs", cuidOpt.get.toString, pveName, "pve", "bin", "python")
if (Files.exists(candidate) && Files.isExecutable(candidate)) candidate.toString
else fallback
}
```
Not a correctness issue; drop if you don't care.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -140,6 +147,7 @@ class PythonUDFOpDescV2 extends LogicalOp {
.withPartitionRequirement(partitionRequirement)
.withIsOneToManyOp(true)
.withPropagateSchema(SchemaPropagationFunc(propagateSchema))
+ .withPveName(envName.trim)
Review Comment:
Two sentinels currently mean "no PVE": `""` (the `PhysicalOp.pveName` default) and `"Default"` (the magic UI string). `choosePythonBin` handles both, but it's easy to forget one. Normalize at the boundary here:
```scala
.withPveName(if (envName.trim == "Default") "" else envName.trim)
```
Same change applies in `DualInputPortsPythonUDFOpDescV2` and `PythonUDFSourceOpDescV2`.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -434,4 +434,26 @@ object PveManager {
List(s"[PVE][ERR] Failed to delete package for cuid=$cuid: ${e.getMessage}")
}
}
+
+ def getPveNames(cuid: Int): List[String] = {
+
+ val cuPath = Paths.get("/tmp/texera-pve/venvs").resolve(cuid.toString)
+
+ if (!Files.exists(cuPath) || !Files.isDirectory(cuPath)) {
+ return List()
+ }
+
+ val stream = Files.list(cuPath)
+
+ try {
+ stream
+ .iterator()
+ .asScala
+ .filter(path => Files.isDirectory(path))
+ .map(path => path.getFileName.toString)
+ .toList
+ } finally {
+ stream.close()
+ }
+ }
Review Comment:
This duplicates `getEnvironments(cuid)`. The existing method already returns `pveName` (just along with `userPackages`). The frontend could `.map(p => p.pveName)`, and you'd avoid maintaining a second definition of "what counts as an environment." If you do keep this, at least implement it as `getEnvironments(cuid).map(_.pveName)` so the two methods don't drift. The current implementation lists any directory under `cuid/` including unrelated files, while `getEnvironments` doesn't validate venv structure either (checking that `dir/pve/bin/python` exists).
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -173,9 +177,31 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On
private changeDetectorRef: ChangeDetectorRef,
private workflowVersionService: WorkflowVersionService,
private workflowStatusSerivce: WorkflowStatusService,
- private config: GuiConfigService
+ private config: GuiConfigService,
+ private workflowPveService: WorkflowPveService,
+ private computingUnitStatusService: ComputingUnitStatusService
) {}
+ private patchPythonUdfEnvironmentSchema(schema: CustomJSONSchema7, environments: string[]): CustomJSONSchema7 {
+ const patchedSchema = cloneDeep(schema);
+
+ if (patchedSchema.properties && typeof patchedSchema.properties !== "boolean") {
+ const envOptions = ["Default", ...environments.filter(e => e !== "Default")];
+
+ if (!patchedSchema.properties["envName"]) {
+ patchedSchema.properties["envName"] = { type: "string" };
+ }
Review Comment:
Dead branch. The backend always emits `envName` in the schema for these three operator types. Either drop this fallback and let a missing property throw or convert to an assertion.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResource.scala:
##########
@@ -113,4 +113,14 @@ class PveResource {
Response.ok(messages.asJava).build()
}
}
+
+ // --------------------------------------------------
+ // Get list of existing environments for a CU
+ // --------------------------------------------------
+ @GET
+ @Path("/environments")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def getPveNames(@QueryParam("cuid") cuid: Int): java.util.List[String] = {
Review Comment:
Suggested by Claude:
`cuid: Int` compiles to the JVM primitive `int`, which can't be null. When the request has no `cuid` (or it's blank), JAX-RS has no way to represent "absent" and falls back to the primitive default `0`. So:
| Request | `cuid` value | Behavior |
|---|---|---|
| `GET /pve/environments?cuid=42` | `42` | Correct |
| `GET /pve/environments` *(missing)* | `0` | Looks up `/tmp/texera-pve/venvs/0/` |
| `GET /pve/environments?cuid=` *(empty)* | `0` | Same |
**Why this matters in practice:**
1. CU ids are auto-incremented in the DB starting from 1, so `0` is currently unused — but the moment anyone seeds CU `0` (test fixture, manual SQL, dev reset), that user's PVE list leaks to any request missing the param.
2. The new frontend caller at `operator-property-edit-frame.component.ts` already guards with `cuid !== undefined ? ... : of([])`. That guard exists in *one* place. The next caller (a curl, a dev script, a future component) won't have it, and the API silently does the wrong thing instead of returning 400 — that's the kind of bug that gets noticed in prod, not review.
3. Returning `[]` for `cuid=0` is also ambiguous with "this CU exists but has no PVEs" — a real 400 disambiguates client-side error handling.
**Fix — switch to the boxed type and reject null explicitly:**
```scala
@GET
@Path("/environments")
@Produces(Array(MediaType.APPLICATION_JSON))
def getPveNames(@QueryParam("cuid") cuid: java.lang.Integer): Response = {
if (cuid == null) {
Response.status(Response.Status.BAD_REQUEST)
.entity("cuid query parameter is required").build()
} else {
Response.ok(PveManager.getPveNames(cuid).asJava).build()
}
}
```
`java.lang.Integer` is a reference type, so JAX-RS passes `null` when the param is absent and you can branch on it. (You can also use `@DefaultValue("-1")` + an explicit `< 0` check, but boxed-null reads more clearly as "required parameter".)
**Same bug already exists in `fetchPVEs` at line 64 of this file** — it also takes `@QueryParam("cuid") cuid: Int` and would silently look up CU `0` on a missing param. Not part of this diff, but worth fixing in the same commit since both endpoints share the failure mode (and `getPveNames` was modeled on `fetchPVEs`). If you fix both, consider extracting a small helper:
```scala
private def requireCuid(cuid: java.lang.Integer): Int = {
if (cuid == null) throw new BadRequestException("cuid query parameter is required")
cuid
}
```
Then each endpoint reads `val id = requireCuid(cuid)` at the top.
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -249,6 +275,45 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On
*/
this.formData = cloneDeep(operator.operatorProperties);
+ // Pre-fill the PVE selection to "Default" for freshly-dropped Python UDF operators.
+ // AJV's useDefaults pass below runs against the unpatched schema, so relying on the
+ // patched-schema default isn't enough; do it directly on formData here. Existing
+ // operators that already have an envName keep their value.
+ const isPythonUdf =
+ this.currentOperatorSchema.operatorType === "PythonUDFV2" ||
+ this.currentOperatorSchema.operatorType === "DualInputPortsPythonUDFV2" ||
+ this.currentOperatorSchema.operatorType === "PythonUDFSourceV2";
+ if (isPythonUdf && !this.formData.envName) {
+ this.formData.envName = "Default";
+ }
+
+ const baseSchema = cloneDeep(this.currentOperatorSchema.jsonSchema);
+
+ if (isPythonUdf) {
+ this.computingUnitStatusService
+ .getSelectedComputingUnit()
+ .pipe(
+ take(1),
+ switchMap(unit => {
+ const cuid = unit?.computingUnit?.cuid;
+ return cuid !== undefined ? this.workflowPveService.getPveNames(cuid) : of<string[]>([]);
+ }),
+ untilDestroyed(this)
+ )
+ .subscribe({
+ next: (environments: string[]) => {
+ const patchedSchema = this.patchPythonUdfEnvironmentSchema(baseSchema, environments);
+ this.setFormlyFormBinding(patchedSchema);
+ },
+ error: (err: unknown) => {
+ const patchedSchema = this.patchPythonUdfEnvironmentSchema(baseSchema, []);
+ this.setFormlyFormBinding(patchedSchema);
+ },
Review Comment:
The `err` is unused, and the user gets a dropdown with only `Default` whether the request failed or genuinely returned `[]`. At minimum `console.error(err)` or use `NotificationService` so users can tell their PVEs aren't loading vs. don't exist.
--
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: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 merged PR #5069:
URL: https://github.com/apache/texera/pull/5069
--
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: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
Posted by "SarahAsad23 (via GitHub)" <gi...@apache.org>.
SarahAsad23 commented on code in PR #5069:
URL: https://github.com/apache/texera/pull/5069#discussion_r3262531961
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -140,6 +147,7 @@ class PythonUDFOpDescV2 extends LogicalOp {
.withPartitionRequirement(partitionRequirement)
.withIsOneToManyOp(true)
.withPropagateSchema(SchemaPropagationFunc(propagateSchema))
+ .withPveName(envName.trim)
Review Comment:
This concern has been addressed by adding a checkbox that users can select when they want to use the default 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: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5069:
URL: https://github.com/apache/texera/pull/5069#issuecomment-4455945638
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5069?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 42.83%. Comparing base ([`7bb102e`](https://app.codecov.io/gh/apache/texera/commit/7bb102e8febbdbdbae116f02537189804bd1b7b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5461194`](https://app.codecov.io/gh/apache/texera/commit/5461194141b4342ed76f4190736259a79f2e3ba3?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 #5069 +/- ##
============================================
- Coverage 42.85% 42.83% -0.02%
+ Complexity 2207 2203 -4
============================================
Files 1045 1045
Lines 40146 40126 -20
Branches 4240 4240
============================================
- Hits 17203 17187 -16
+ Misses 21878 21873 -5
- Partials 1065 1066 +1
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5069/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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.53% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.72% <ø> (ø)` | | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.70% <ø> (-0.05%)` | :arrow_down: | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.18% <ø> (ø)` | | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.92% <ø> (-0.01%)` | :arrow_down: | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `88.99% <ø> (ø)` | | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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/5069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.72% <ø> (ø)` | | Carriedforward from [ac55403](https://app.codecov.io/gh/apache/texera/commit/ac5540359290afebee35889f47342ff296a875cb?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 Sentry](https://app.codecov.io/gh/apache/texera/pull/5069?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>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :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: Add Python Virtual Environment Support: Execute Python UDFs Inside PVEs [texera]
Posted by "kunwp1 (via GitHub)" <gi...@apache.org>.
kunwp1 commented on code in PR #5069:
URL: https://github.com/apache/texera/pull/5069#discussion_r3269884814
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/source/PythonUDFSourceOpDescV2.scala:
##########
@@ -49,6 +49,18 @@ class PythonUDFSourceOpDescV2 extends SourceOperatorDescriptor {
@JsonPropertyDescription("Specify how many parallel workers to launch")
var workers: Int = 1
+ @JsonProperty(required = true, defaultValue = "true")
+ @JsonSchemaTitle("Default Python Environment")
+ @JsonPropertyDescription("Use Default Python Environment\"")
Review Comment:
Is adding `\"` a typo? Happening to all the operator descriptors.
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -173,9 +177,29 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On
private changeDetectorRef: ChangeDetectorRef,
private workflowVersionService: WorkflowVersionService,
private workflowStatusSerivce: WorkflowStatusService,
- private config: GuiConfigService
+ private config: GuiConfigService,
+ private workflowPveService: WorkflowPveService,
+ private computingUnitStatusService: ComputingUnitStatusService
) {}
+ private patchPythonUdfEnvironmentSchema(schema: CustomJSONSchema7, environments: string[]): CustomJSONSchema7 {
+ const patchedSchema = cloneDeep(schema);
+
+ if (patchedSchema.properties && typeof patchedSchema.properties !== "boolean") {
+ const envProperty = patchedSchema.properties["envName"] as CustomJSONSchema7;
+ envProperty.enum = environments;
+ }
+
+ return patchedSchema;
+ }
+
+ private hideEnvNameWhenDefaultEnvChecked(): void {
+ const envField = this.formlyFields?.[0]?.fieldGroup?.find(f => f.key === "envName");
+ if (envField) {
+ envField.expressions = { hide: "!!field.parent.model.defaultEnv" };
Review Comment:
Add `required: true` on `envName` when `defaultEnv` is `false`
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -140,6 +152,7 @@ class PythonUDFOpDescV2 extends LogicalOp {
.withPartitionRequirement(partitionRequirement)
.withIsOneToManyOp(true)
.withPropagateSchema(SchemaPropagationFunc(propagateSchema))
+ .withPveName(if (defaultEnv) "" else envName.trim)
Review Comment:
ditto
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/DualInputPortsPythonUDFOpDescV2.scala:
##########
@@ -129,6 +141,7 @@ class DualInputPortsPythonUDFOpDescV2 extends LogicalOp {
Map(operatorInfo.outputPorts.head.id -> outputSchema)
})
)
+ .withPveName(if (defaultEnv) "" else envName.trim)
Review Comment:
ditto
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/source/PythonUDFSourceOpDescV2.scala:
##########
@@ -68,6 +80,7 @@ class PythonUDFSourceOpDescV2 extends SourceOperatorDescriptor {
SchemaPropagationFunc(_ => Map(operatorInfo.outputPorts.head.id -> sourceSchema()))
)
.withLocationPreference(Option.empty)
+ .withPveName(if (defaultEnv) "" else envName.trim)
Review Comment:
If a user unchecks "Default Python Environment" but leaves envName empty, the form silently submits and `withPveName(if (defaultEnv) "" else envName.trim)` resolves to `""` which falls back to default Python. Can you add a check to handle the case where `defaultEnv: false, envName: ""`?
--
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