fgerlits commented on code in PR #1721:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1512842925
##########
extensions/python/PYTHON.md:
##########
@@ -155,10 +158,33 @@ In the flow configuration these Python processors can be
referenced by their ful
Due to some differences between the NiFi and MiNiFi C++ processors and
implementation, there are some limitations using the NiFi Python processors:
- Record based processors are not yet supported in MiNiFi C++, so the NiFi
Python processors inherited from RecordTransform are not supported.
-- Virtualenv support is not yet available in MiNiFi C++, so all required
packages must be installed on the system.
- Controller properties are not supported at the moment.
- There are some validators in NiFi that are not present in MiNiFi C++, so
some property validations will be missing using the NiFi Python processors.
- Allowable values specified in NiFi Python processors are ignored in MiNiFi
C++ (due to MiNiFi C++ requiring them to be specified at compile time), so the
property values are not pre-verified.
- MiNiFi C++ only supports expression language with flow file attributes, so
only FLOWFILE_ATTRIBUTES expression language scope is supported, otherwise the
expression language will not be evaluated.
- MiNiFi C++ does not support property dependencies, so the property
dependencies will be ignored. If a property depends on another property, the
property will not be required.
- MiNiFi C++ does not support the use of self.jvm member in Python processors
that provides JVM bindings in NiFi, it is set to None in MiNiFi C++.
+- Inline definition of Python package dependencies, defined in the
ProcessorDetails nested class are not supported as in NiFi, so the dependencies
must be defined in the requirements.txt files. If a processor's dependencies
are defined in the ProcessorDetails class, the dependencies should be copied to
the requirements.txt file.
+
+## Use Python processors from virtualenv
+
+It is possible to set a virtualenv to be used by the Python processors in
Apache MiNiFi C++. If the virtualenv directory is set, the Python processors
will be executed using the packages installed in the virtualenv. If the
virtualenv directory is not set, the Python processors will be executed using
the packages installed on the system.
+
+ # in minifi.properties
+ nifi.python.virtualenv.directory=${MINIFI_HOME}/minifi-python-env
+
+**NOTE:* Using different python versions for the system and the virtualenv is
not supported. The virtualenv must be created using the same python version as
the system python.
+
+## Automatically install dependencies from requirements.txt files
+
+It is possible to automatically install the dependencies of the Python
processors in the virtualenv defined in requirements.txt files. To enable this
feature, the `nifi.python.install.packages.automatically` property must be set
to true. If this property is set to true, then all requirements.txt files that
appear under the MiNiFi Python directory and its subdirectories (defined by the
`nifi.python.processor.dir` property) will be used to install the Python
packages. If the `nifi.python.virtualenv.directory` property is set, the
packages are installed in the virtualenv, otherwise this option is ignored. Due
to install schema differences in different platforms, system level packages are
expected to be installed manually by the user.
Review Comment:
I find "... otherwise this option is ignored" unclear (which option is
"this"?) If it means that for automatic installation, both properties need to
be set, I would write:
```suggestion
It is possible to automatically install the dependencies of the Python
processors defined in requirements.txt files into a virtualenv. To enable this
feature, the `nifi.python.install.packages.automatically` property must be set
to true, and the `nifi.python.virtualenv.directory` property must be set to a
directory where a virtualenv either already exists, or it can be set up. In
this case, all requirements.txt files that appear under the MiNiFi Python
directory (defined by the `nifi.python.processor.dir` property) and its
subdirectories will be used to install the Python packages into the given
virtualenv. Due to install schema differences in different platforms, system
level packages are expected to be installed manually by the user.
```
##########
extensions/python/PythonScriptEngine.cpp:
##########
@@ -68,6 +67,73 @@ void initThreads() {
#pragma warning(pop)
#endif
}
+
+std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) {
+#if WIN32
+ return "\"" + command + "\"";
+#else
+ return command;
+#endif
+}
+
+std::vector<std::filesystem::path> getRequirementsFilePaths(const
std::shared_ptr<Configure> &configuration) {
+ std::vector<std::filesystem::path> paths;
+ if (auto python_processor_path =
configuration->get(minifi::Configuration::nifi_python_processor_dir)) {
+ for (const auto& entry :
std::filesystem::recursive_directory_iterator(std::filesystem::path{*python_processor_path}))
{
+ if (std::filesystem::is_regular_file(entry.path()) &&
entry.path().filename() == "requirements.txt") {
+ paths.push_back(entry.path());
+ }
+ }
+ }
+ return paths;
+}
+
+std::string getPythonBinary(const std::shared_ptr<Configure> &configuration) {
+#if WIN32
+ std::string python_binary = "python";
+#else
+ std::string python_binary = "python3";
+#endif
+ if (auto binary =
configuration->get(minifi::Configuration::nifi_python_env_setup_binary)) {
+ python_binary = *binary;
+ }
+ return python_binary;
+}
+
+void createVirtualEnvIfSpecified(const std::shared_ptr<Configure>
&configuration) {
+ if (auto path =
configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) {
+ PythonConfigState::getInstance().virtualenv_path = *path;
+ if
(!std::filesystem::exists(PythonConfigState::getInstance().virtualenv_path) ||
!std::filesystem::is_empty(PythonConfigState::getInstance().virtualenv_path)) {
Review Comment:
why is the virtualenv creation skipped if the directory is empty?
##########
extensions/python/PythonConfigState.h:
##########
@@ -0,0 +1,67 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <vector>
+#include <utility>
+#include <algorithm>
+#include <string>
+#include <memory>
+#include <filesystem>
+#include "core/Core.h"
+#include "core/logging/LoggerConfiguration.h"
+#include "core/Resource.h"
+#include "ExecutePythonProcessor.h"
+#include "PythonObjectFactory.h"
+#include "agent/agent_version.h"
+#include "agent/build_description.h"
+#include "utils/file/FileUtils.h"
+#include "utils/StringUtils.h"
+#include "range/v3/algorithm.hpp"
+#include "properties/Configuration.h"
+#include "utils/file/FilePattern.h"
+#include "range/v3/view/filter.hpp"
Review Comment:
are all these `#include`s needed in this header file?
##########
docker/test/integration/cluster/containers/MinifiContainer.py:
##########
@@ -33,7 +33,11 @@ def __init__(self):
self.enable_prometheus = False
self.enable_prometheus_with_ssl = False
self.enable_sql = False
- self.use_nifi_python_processors = False
+ self.use_nifi_python_processors_with_system_python_packages_installed
= False
+ self.use_nifi_python_processors_with_virtualenv = False
+ self.use_nifi_python_processors_with_virtualenv_packages_installed =
False
+ self.create_python_virtualenv = False
+ self.install = False
Review Comment:
Where are these two used?
##########
docker/python-verify/conda.Dockerfile:
##########
@@ -31,14 +31,12 @@ USER root
RUN wget https://repo.anaconda.com/archive/Anaconda3-2023.09-0-Linux-x86_64.sh
-P /tmp \
&& echo "6c8a4abb36fbb711dc055b7049a23bbfd61d356de9468b41c5140f8a11abd851
/tmp/Anaconda3-2023.09-0-Linux-x86_64.sh" | sha256sum -c \
&& bash /tmp/Anaconda3-2023.09-0-Linux-x86_64.sh -b -p /opt/conda \
- && chown -R ${USER}:${USER} /opt/conda \
- && mkdir /home/${USER} \
- && chown -R ${USER}:${USER} /home/${USER}
+ && chown -R ${USER}:${USER} /opt/conda
USER ${USER}
RUN ${CONDA_HOME}/bin/conda init bash
-RUN ${CONDA_HOME}/bin/conda install langchain -c conda-forge
+RUN ${CONDA_HOME}/bin/conda install "langchain<=0.17.0" -c conda-forge
Review Comment:
Why `<= 0.17.0`? A comment could be useful so we can remove this restriction
when it is no longer needed.
##########
extensions/python/PythonScriptEngine.cpp:
##########
@@ -68,6 +67,73 @@ void initThreads() {
#pragma warning(pop)
#endif
}
+
+std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) {
+#if WIN32
+ return "\"" + command + "\"";
+#else
+ return command;
+#endif
+}
+
+std::vector<std::filesystem::path> getRequirementsFilePaths(const
std::shared_ptr<Configure> &configuration) {
+ std::vector<std::filesystem::path> paths;
+ if (auto python_processor_path =
configuration->get(minifi::Configuration::nifi_python_processor_dir)) {
+ for (const auto& entry :
std::filesystem::recursive_directory_iterator(std::filesystem::path{*python_processor_path}))
{
+ if (std::filesystem::is_regular_file(entry.path()) &&
entry.path().filename() == "requirements.txt") {
+ paths.push_back(entry.path());
+ }
+ }
+ }
+ return paths;
+}
+
+std::string getPythonBinary(const std::shared_ptr<Configure> &configuration) {
+#if WIN32
+ std::string python_binary = "python";
+#else
+ std::string python_binary = "python3";
+#endif
+ if (auto binary =
configuration->get(minifi::Configuration::nifi_python_env_setup_binary)) {
+ python_binary = *binary;
+ }
+ return python_binary;
+}
+
+void createVirtualEnvIfSpecified(const std::shared_ptr<Configure>
&configuration) {
+ if (auto path =
configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) {
+ PythonConfigState::getInstance().virtualenv_path = *path;
+ if
(!std::filesystem::exists(PythonConfigState::getInstance().virtualenv_path) ||
!std::filesystem::is_empty(PythonConfigState::getInstance().virtualenv_path)) {
+ auto venv_command = "\"" +
PythonConfigState::getInstance().python_binary + "\" -m venv \"" +
PythonConfigState::getInstance().virtualenv_path.string() + "\"";
+ auto return_value =
std::system(encapsulateCommandInQuotesIfNeeded(venv_command).c_str());
+ if (return_value != 0) {
+ throw PythonScriptException(fmt::format("The following command
creating python virtual env failed: '{}'", venv_command));
+ }
+ }
+ }
+}
+
+void installPythonPackagesIfRequested(const std::shared_ptr<Configure>
&configuration, const std::shared_ptr<core::logging::Logger>& logger) {
+ std::string automatic_install_str;
+ if (!PythonConfigState::getInstance().isPackageInstallationNeeded()) {
+ return;
+ }
+ auto requirement_file_paths = getRequirementsFilePaths(configuration);
+ for (const auto& requirements_file_path : requirement_file_paths) {
+ logger->log_info("Installing python packages from the following
requirements.txt file: {}", requirements_file_path.string());
+ std::string pip_command;
+#if WIN32
+
pip_command.append("\"").append((PythonConfigState::getInstance().virtualenv_path
/ "Scripts" / "activate.bat").string()).append("\" && ");
+#else
+ pip_command.append(".
\"").append((PythonConfigState::getInstance().virtualenv_path / "bin" /
"activate").string()).append("\" && ");
+#endif
+
pip_command.append("\"").append(PythonConfigState::getInstance().python_binary).append("\"
-m pip install --no-cache-dir -r
\"").append(requirements_file_path.string()).append("\"");
+ auto return_value =
std::system(encapsulateCommandInQuotesIfNeeded(pip_command).c_str());
Review Comment:
Do we need `encapsulateCommandInQuotesIfNeeded`? There are already quotes
around the executable path-and-filename above. In fact, is it going to work?
We'll end up with something like `""\venv_path\Scripts\activate.bat" &&
"python" -m pip install --no-cache-dir -r
"\requirements_path\requirements.txt""`, ie. double quotes at the beginning and
the end, which looks strange.
--
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]
