Yicong-Huang commented on code in PR #5613:
URL: https://github.com/apache/texera/pull/5613#discussion_r3393640740
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -94,26 +96,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")
Review Comment:
can we call it pyamber packages?
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -94,26 +96,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] =
Review Comment:
requirements.txt may not be part of docker image. @bobbai00 can you confirm?
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -94,26 +96,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 = tempVenv.resolve("bin").resolve("python").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,
Review Comment:
Will all of these system calls work across different OSs, especially on
Windows?
I am a bit concerned about putting too much OS-specific process logic
directly in Scala. For example, commands for creating a virtual environment,
activating it, setting environment variables, path handling, and shell syntax
can behave differently between Linux/macOS and Windows.
My suggestion is to move the venv creation logic into scripts and let Scala
invoke the appropriate script based on the OS. For example, we can provide one
Unix shell script and one Windows script:
* create_venv.sh for Linux/macOS
* create_venv.bat or create_venv.ps1 for Windows
Then the Scala code only needs to detect the OS and call the corresponding
script. This keeps the Scala side simpler and makes the platform-specific
behavior easier to test and maintain.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html:
##########
@@ -472,28 +472,40 @@
</ng-template>
<nz-collapse-panel [nzHeader]="systemHeaderTpl">
<div class="system-panel-inner">
- <div class="package-header-row">
- <div class="package-column-label">Package</div>
- <div class="package-column-label">Version</div>
+ <div
+ *ngIf="systemPackagesLoading"
+ class="system-loading">
+ <span
+ nz-icon
+ nzType="loading"
+ nzTheme="outline"></span>
+ Fetching system packages…
</div>
- <div
- *ngFor="let pkg of systemPackages"
- class="package-row system-row">
- <div class="system-package-inputs">
- <input
- nz-input
- class="system-input"
- [disabled]="true"
- [ngModel]="pkg.name" />
+ <ng-container *ngIf="!systemPackagesLoading">
+ <div class="package-header-row">
+ <div class="package-column-label">Package</div>
+ <div class="package-column-label">Version</div>
Review Comment:
why are UI changes related to remove system-requirements-locks.txt?
##########
frontend/src/styles.scss:
##########
@@ -161,6 +161,15 @@ hr {
gap: 6px;
}
+ .system-loading {
+ display: flex;
+ align-items: center;
+ gap: 8px;
+ padding: 12px 4px;
+ color: rgba(0, 0, 0, 0.55);
+ font-style: italic;
+ }
Review Comment:
I am confused. is this related?
##########
amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveManager.scala:
##########
@@ -94,26 +96,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-")
Review Comment:
`pyamber-venv`?
--
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]
