exceptionfactory commented on code in PR #8573:
URL: https://github.com/apache/nifi/pull/8573#discussion_r1550777551
##########
nifi-nar-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -224,6 +226,10 @@ private String generateAuthToken() {
return Base64.getEncoder().encodeToString(bytes);
}
+ private boolean isUseVirtualEnv() {
Review Comment:
It looks like the only use of this method is a negated reference. Can this
be removed and replaced with `if (packagedWithDependencies)`?
##########
nifi-nar-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -234,8 +240,18 @@ private Process launchPythonProcess(final int
listeningPort, final String authTo
final List<String> commands = new ArrayList<>();
commands.add(pythonCommand);
+ if (!isUseVirtualEnv()) {
+ // If not using venv, we will not launch a separate virtual
environment, so we need to use the -S
+ // flag in order to prevent the Python process from using the
installation's site-packages. This provides
+ // proper dependency isolation to the Python process.
+ commands.add("-S");
+ }
String pythonPath = pythonApiDirectory.getAbsolutePath();
+ final String absolutePath = virtualEnvHome.getAbsolutePath();
+ final File dependenciesDir = new File(new File(absolutePath),
"NAR-INF/bundled-dependencies");
Review Comment:
Is the intent to add `NAR-INF/bundled-dependencies` in all cases? It seems
like it should be conditional on whether the Processor is packaged with
dependencies.
##########
nifi-nar-bundles/nifi-py4j-bundle/nifi-py4j-bridge/src/main/java/org/apache/nifi/py4j/PythonProcess.java:
##########
@@ -287,6 +309,13 @@ String resolvePythonCommand() throws IOException {
private void setupEnvironment() throws IOException {
+ // Environment creation is only necessary if using PIP. Otherwise, the
Process requires no outside dependencies, other than those
+ // provided in the package and thus we can simply include those
packages in the PYTHON_PATH.
+ if (!isUseVirtualEnv()) {
+ logger.debug("Will not create Python Virtual Environment because
PIP is disabled in nifi.properties");
Review Comment:
Very minor, but general usage of `pip` is all lowercase and opposed to all
uppercase. More importantly though, the message does not reflect the behavior,
as it is only based on the the presence of packaged dependencies.
```suggestion
logger.debug("Python Virtual Environment not created: Python
Processor packaged with dependencies");
```
--
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]
