Copilot commented on code in PR #5613:
URL: https://github.com/apache/texera/pull/5613#discussion_r3406949278
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -103,26 +114,95 @@ object PveManager {
}
}
- private def getSystemPath(isLocal: Boolean): Path = {
- Paths.get(
- if (isLocal) "amber/system-requirements-lock.txt"
- else "/tmp/system-requirements-lock.txt"
- )
- }
+ private def locateRequirementsTxt(): Option[Path] =
+ Seq(Paths.get("/tmp", "requirements.txt"), Paths.get("amber",
"requirements.txt"))
+ .find(Files.exists(_))
Review Comment:
The PR removes `system-requirements-lock.txt`, but the container build still
references it (e.g., `bin/computing-unit-master.dockerfile` copies
`/texera/amber/system-requirements-lock.txt` to `/tmp`). Without updating that
build artifact, Docker builds will fail due to a missing source file.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -103,26 +114,95 @@ object PveManager {
}
}
- private def getSystemPath(isLocal: Boolean): Path = {
- Paths.get(
- if (isLocal) "amber/system-requirements-lock.txt"
- else "/tmp/system-requirements-lock.txt"
- )
- }
+ private def locateRequirementsTxt(): Option[Path] =
+ Seq(Paths.get("/tmp", "requirements.txt"), Paths.get("amber",
"requirements.txt"))
+ .find(Files.exists(_))
+
+ // Resolves the fully-pinned system package set by installing
requirements.txt
+ // into a throwaway venv and running `pip freeze`.
+ private def resolveSystemPackages(): Seq[String] = {
+ val requirementsPath = locateRequirementsTxt() match {
+ case Some(p) => p
+ case None =>
+ logger.error("requirements.txt not found; system package set will be
empty")
+ return Seq.empty
+ }
- def getSystemPackages(isLocal: Boolean): Seq[String] = {
- if (!Files.exists(getSystemPath(isLocal))) {
- Seq()
- } else {
- Files
- .readAllLines(getSystemPath(isLocal))
- .asScala
- .map(_.trim)
- .filter(line => line.nonEmpty && !line.startsWith("#"))
- .toSeq
+ val tempVenv = Files.createTempDirectory("texera-system-venv-")
+ try {
+ val python = venvPython(tempVenv).toString
+ val createCode =
+ Process(Seq(PythonUtils.getPythonExecutable, "-m", "venv",
tempVenv.toString)).!
Review Comment:
`resolveSystemPackages()` creates a temp venv using `Process(...).!` without
a `ProcessLogger`, so any output goes directly to the process stdout/stderr
(and failures lack context). This makes failures harder to diagnose and can
spam server logs/console.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -103,26 +114,95 @@ object PveManager {
}
}
- private def getSystemPath(isLocal: Boolean): Path = {
- Paths.get(
- if (isLocal) "amber/system-requirements-lock.txt"
- else "/tmp/system-requirements-lock.txt"
- )
- }
+ private def locateRequirementsTxt(): Option[Path] =
+ Seq(Paths.get("/tmp", "requirements.txt"), Paths.get("amber",
"requirements.txt"))
+ .find(Files.exists(_))
+
+ // Resolves the fully-pinned system package set by installing
requirements.txt
+ // into a throwaway venv and running `pip freeze`.
+ private def resolveSystemPackages(): Seq[String] = {
+ val requirementsPath = locateRequirementsTxt() match {
+ case Some(p) => p
+ case None =>
+ logger.error("requirements.txt not found; system package set will be
empty")
+ return Seq.empty
+ }
- def getSystemPackages(isLocal: Boolean): Seq[String] = {
- if (!Files.exists(getSystemPath(isLocal))) {
- Seq()
- } else {
- Files
- .readAllLines(getSystemPath(isLocal))
- .asScala
- .map(_.trim)
- .filter(line => line.nonEmpty && !line.startsWith("#"))
- .toSeq
+ val tempVenv = Files.createTempDirectory("texera-system-venv-")
+ try {
+ val python = venvPython(tempVenv).toString
+ val createCode =
+ Process(Seq(PythonUtils.getPythonExecutable, "-m", "venv",
tempVenv.toString)).!
+ if (createCode != 0) {
+ logger.error(s"failed to create temp venv for system-package
resolution (exit=$createCode)")
+ return Seq.empty
+ }
+
+ val installCode = Process(
+ Seq(
+ python,
+ "-u",
+ "-m",
+ "pip",
+ "install",
+ "--progress-bar",
+ "off",
+ "--no-input",
+ "-r",
+ requirementsPath.toString
+ ),
+ None,
+ pipEnv.toSeq: _*
+ ).!
+ if (installCode != 0) {
+ logger.error(s"failed to install requirements into temp venv
(exit=$installCode)")
+ return Seq.empty
+ }
+
+ val collected = scala.collection.mutable.ListBuffer[String]()
+ val freezeCode = Process(Seq(python, "-m", "pip", "freeze")).!(
+ ProcessLogger(line => collected += line, _ => ())
+ )
Review Comment:
`pip freeze` currently discards stderr (`_ => ()`). If `pip freeze` fails or
emits warnings/errors, they won’t be visible anywhere, which makes diagnosis
difficult.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -103,26 +114,95 @@ object PveManager {
}
}
- private def getSystemPath(isLocal: Boolean): Path = {
- Paths.get(
- if (isLocal) "amber/system-requirements-lock.txt"
- else "/tmp/system-requirements-lock.txt"
- )
- }
+ private def locateRequirementsTxt(): Option[Path] =
+ Seq(Paths.get("/tmp", "requirements.txt"), Paths.get("amber",
"requirements.txt"))
+ .find(Files.exists(_))
+
+ // Resolves the fully-pinned system package set by installing
requirements.txt
+ // into a throwaway venv and running `pip freeze`.
+ private def resolveSystemPackages(): Seq[String] = {
+ val requirementsPath = locateRequirementsTxt() match {
+ case Some(p) => p
+ case None =>
+ logger.error("requirements.txt not found; system package set will be
empty")
+ return Seq.empty
+ }
- def getSystemPackages(isLocal: Boolean): Seq[String] = {
- if (!Files.exists(getSystemPath(isLocal))) {
- Seq()
- } else {
- Files
- .readAllLines(getSystemPath(isLocal))
- .asScala
- .map(_.trim)
- .filter(line => line.nonEmpty && !line.startsWith("#"))
- .toSeq
+ val tempVenv = Files.createTempDirectory("texera-system-venv-")
+ try {
+ val python = venvPython(tempVenv).toString
+ val createCode =
+ Process(Seq(PythonUtils.getPythonExecutable, "-m", "venv",
tempVenv.toString)).!
+ if (createCode != 0) {
+ logger.error(s"failed to create temp venv for system-package
resolution (exit=$createCode)")
+ return Seq.empty
+ }
+
+ val installCode = Process(
+ Seq(
+ python,
+ "-u",
+ "-m",
+ "pip",
+ "install",
+ "--progress-bar",
+ "off",
+ "--no-input",
+ "-r",
+ requirementsPath.toString
+ ),
+ None,
+ pipEnv.toSeq: _*
+ ).!
Review Comment:
`pip install` in `resolveSystemPackages()` runs without capturing
stdout/stderr (uses `. !` without a `ProcessLogger`). If it fails, we only get
an exit code and may lose the actual pip error output that would be needed for
debugging.
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -19,6 +19,9 @@
package org.apache.texera.web.resource.pythonvirtualenvironment
+import com.typesafe.scalalogging.LazyLogging
+import org.apache.commons.lang3.SystemUtils
+
Review Comment:
`PveManager` now depends on `org.apache.commons.lang3.SystemUtils`, but this
PR doesn’t add an explicit `commons-lang3` dependency in sbt. If it’s not
already on the compile classpath, this will fail to compile; even if it’s
currently transitive, relying on it implicitly is brittle. Consider either
adding an explicit dependency or switching to a JDK-based OS check (e.g.,
`java.io.File.separatorChar`).
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -401,8 +462,8 @@ object PveManager {
val code = runPipInstall(
python,
Seq(
- "--constraint", // check against system-requirements-lock
- getSystemPath(isLocal).toString,
+ "--constraint", // pin to the runtime-resolved system set
+ systemConstraintFile.toString,
Review Comment:
If resolving the system package set fails (e.g., transient pip/network
failure), `systemPackages` becomes empty and we still proceed with user
installs using an effectively empty `--constraint` file. That’s a fail-open
behavior that can allow user installs to overwrite system deps (the exact thing
this check is meant to prevent). Consider aborting user installs when
system-package resolution fails.
--
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]
