Conversation
marc-lehner
requested review from
knime-ghub-bot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR improves Python environment startup and preference testing reliability in the KNIME Python integration, particularly addressing Windows-specific DLL lookup issues (Python 3.12+) and making the Python kernel tester more robust.
Changes:
- Add a
SimplePythonCommand#createProcessBuilder()override to prepend relevant environment directories toPATH(intended to fix Windows conda/pixi DLL resolution issues). - Prevent potential subprocess I/O deadlocks in
PythonKernelTesterby readingstdoutandstderrconcurrently. - Fix argument passing for required additional modules (was incorrectly using optional modules) and modernize the kernel tester script by removing the deprecated
impcheck.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
org.knime.python3/src/main/java/org/knime/python3/SimplePythonCommand.java |
Prepends inferred env directories to PATH when creating a ProcessBuilder. |
org.knime.python3.scripting.nodes/src/main/java/org/knime/python3/scripting/nodes/prefs/PythonKernelTester.java |
Uses canonical script path, fixes module arg wiring, and reads process output concurrently. |
org.knime.python3.scripting.nodes/python-kernel-tester/PythonKernelTester.py |
Removes deprecated imp module requirement check. |
| public ProcessBuilder createProcessBuilder() { | ||
| final var pb = super.createProcessBuilder(); | ||
| final Path executableDir = getExecutablePath().getParent(); | ||
| if (executableDir != null) { | ||
| final var env = pb.environment(); | ||
| // ProcessBuilder.environment() on Windows stores keys case-insensitively but | ||
| // preserves the original casing. Find the existing key to avoid duplicates. | ||
| final String pathKey = env.keySet().stream() // | ||
| .filter(k -> k.equalsIgnoreCase("PATH")) // | ||
| .findFirst() // | ||
| .orElse("PATH"); | ||
| final String existingPath = env.getOrDefault(pathKey, ""); | ||
|
|
||
| // Standard conda/pixi activation paths for Windows, in activation order. | ||
| final var dirsToAdd = new StringBuilder(); | ||
| for (final String subDir : new String[]{"", "Library\\bin", "Library\\mingw-w64\\bin", | ||
| "Library\\usr\\bin", "Scripts"}) { | ||
| final var candidate = subDir.isEmpty() ? executableDir : executableDir.resolve(subDir); | ||
| if (candidate.toFile().isDirectory()) { |
There was a problem hiding this comment.
createProcessBuilder() currently prepends PATH entries on all OSes whenever the executable path has a parent directory. The Javadoc/PR description says this is a Windows-only conda/pixi workaround; consider guarding with a Windows check (e.g., SystemUtils.IS_OS_WINDOWS) and only applying when a conda/pixi layout is detected, otherwise this can unnecessarily alter PATH for non-conda installs and on non-Windows platforms.
| } | ||
| } | ||
| if (dirsToAdd.length() > 0) { | ||
| env.put(pathKey, dirsToAdd + File.pathSeparator + existingPath); |
There was a problem hiding this comment.
When existingPath is empty, env.put(pathKey, dirsToAdd + File.pathSeparator + existingPath) leaves a trailing path separator, which creates an empty PATH element. On Windows an empty PATH element can be interpreted as the current working directory, which is a security risk; build the new PATH so no empty segment is introduced (only append the separator + existingPath when it is non-empty).
| env.put(pathKey, dirsToAdd + File.pathSeparator + existingPath); | |
| final String newPath = existingPath.isEmpty() | |
| ? dirsToAdd.toString() | |
| : dirsToAdd + File.pathSeparator + existingPath; | |
| env.put(pathKey, newPath); |
| final var stderrReader = new Thread(() -> { | ||
| try (var err = process.getErrorStream()) { | ||
| IOUtils.copy(err, errorWriter, StandardCharsets.UTF_8); | ||
| } catch (final IOException e) { | ||
| LOGGER.debug("Error reading stderr of Python kernel tester", e); | ||
| } | ||
| }, "PythonKernelTester-stderr"); | ||
| stderrReader.start(); | ||
| try (var in = process.getInputStream()) { | ||
| IOUtils.copy(in, outputWriter, StandardCharsets.UTF_8); | ||
| } | ||
| try { | ||
| stderrReader.join(); | ||
| } catch (final InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
If reading stdout (IOUtils.copy(in, ...)) throws, the method exits via the outer catch (IOException ...) without joining the stderr thread or ensuring the process/streams are cleaned up, which can leave a background thread running and resources open. Consider joining the stderr reader in a finally block (and/or using an executor) so it is always terminated before returning, even on exceptions.
|
Added a custom override of
createProcessBuilder()inSimplePythonCommandto prepend necessary conda/pixi environment directories to thePATHvariable on Windows. This ensures Python 3.12+ subprocesses start correctly by locating required DLLs, avoiding Windows SxS assembly errors. Only existing directories are prepended to prevent unnecessary pollution ofPATH. (org.knime.python3/src/main/java/org/knime/python3/SimplePythonCommand.java)Improved subprocess output handling in
PythonKernelTester.javaby readingstdoutandstderrconcurrently in separate threads. This avoids potential deadlocks when large outputs are generated, increasing robustness of the Python kernel testing logic. (org.knime.python3.scripting.nodes/src/main/java/org/knime/python3/scripting/nodes/prefs/PythonKernelTester.java)Fixed a bug where additional required modules were not being passed correctly to the Python kernel tester; previously, optional modules were incorrectly used in place of required ones. (
org.knime.python3.scripting.nodes/src/main/java/org/knime/python3/scripting/nodes/prefs/PythonKernelTester.java)Changed the method for obtaining the Python kernel tester script path to use
getCanonicalPath()instead ofgetAbsolutePath(), ensuring correct path resolution in cases involving symbolic links and spaces. (org.knime.python3.scripting.nodes/src/main/java/org/knime/python3/scripting/nodes/prefs/PythonKernelTester.java)Removed the check for the deprecated
impmodule in the Python kernel tester, reflecting modern Python best practices. (org.knime.python3.scripting.nodes/python-kernel-tester/PythonKernelTester.py)