NIFI-12959: Support loading python processors from NARs #8573
Conversation
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
.../nifi-py4j-bundle/nifi-py4j-bridge/src/test/java/org/apache/nifi/py4j/PythonProcessTest.java
Outdated
Show resolved
Hide resolved
|
Nice improvement Mark! Would this approach help with a processor like ParseDocument where Tesseract has to be installed on the node first? |
|
@pvillard31 no, it wouldn't really help there. The issue there is that a native library like Tesseract would need to be compiled for the correct OS and architecture. So in order to include that, we'd need to include many copies of the library. And currently, we do not set any sort of environment variables telling it to search for native libraries in the NAR's unpacked directory. It might be worth exploring as a future improvement, though. Even though it may not help for something like ParseDocument, it might be helpful at least for custom nars, where perhaps you know that you're only going to run in containers or on a specific architecture/os so you can include the appropriate library for the custom nar? |
|
Yeah, I must admit that I tried to figure out a way to use ParseDocument with NiFi in a container and I didn't manage to find a clean way to bring Tesseract into it. I feel like it would probably be easier to have it as a side container or something but I didn't find a way to piece everything together so I was naively (without too much hopes) thinking that this improvement may help :) |
…d nar and instead switched to a dependency that can be easily loaded using download-maven-plugin; also fixed missing new-lines in unit test
|
@dan-s1 thanks great catch. I removed some carrige-return-newline combos in that file and my IDE did something I didn't expect :) Fixed that. |
There was a problem hiding this comment.
Thanks for the improvement @markap14, this will be helpful for Python Processor deployment! I just noted a name change to reflect the Python library used for testing.
...sts/nifi-python-test-extensions-nar/src/main/resources/WriteJson5VersionBundledDependency.py
Outdated
Show resolved
Hide resolved
...ts/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/python/PythonNarIT.java
Outdated
Show resolved
Hide resolved
...ts/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/python/PythonNarIT.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the updates @markap14, I noted a couple more minor questions, otherwise this looks ready to go.
| // 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"); |
There was a problem hiding this 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.
| logger.debug("Will not create Python Virtual Environment because PIP is disabled in nifi.properties"); | |
| logger.debug("Python Virtual Environment not created: Python Processor packaged with dependencies"); |
| @@ -224,6 +226,10 @@ private String generateAuthToken() { | |||
| return Base64.getEncoder().encodeToString(bytes); | |||
| } | |||
|
|
|||
| private boolean isUseVirtualEnv() { | |||
There was a problem hiding this comment.
It looks like the only use of this method is a negated reference. Can this be removed and replaced with if (packagedWithDependencies)?
|
|
||
| String pythonPath = pythonApiDirectory.getAbsolutePath(); | ||
| final String absolutePath = virtualEnvHome.getAbsolutePath(); | ||
| final File dependenciesDir = new File(new File(absolutePath), "NAR-INF/bundled-dependencies"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks for making the adjustments @markap14, the latest version looks good! +1 merging
Summary
NIFI-00000
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation