Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm closed pull request #1721: MINIFICPP-2277 Add virtualenv support for python processors URL: https://github.com/apache/nifi-minifi-cpp/pull/1721 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559539605 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + python_binary_ = getPythonBinary(configuration); + std::string automatic_install_str; + install_python_packages_automatically_ = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); + if (auto path = configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) { +virtualenv_path_ = *path; +logger_->log_debug("Python virtualenv path was specified at: {}", virtualenv_path_.string()); + } else { +logger_->log_debug("No valid python virtualenv path was specified"); + } + if (auto python_processor_dir = configuration->get(minifi::Configuration::nifi_python_processor_dir)) { +python_processor_dir_ = *python_processor_dir; +logger_->log_debug("Python processor dir was specified at: {}", python_processor_dir_.string()); + } else { +logger_->log_debug("No valid python processor dir was not specified in properties"); + } + createVirtualEnvIfSpecified(); + addVirtualenvToPath(); +} + +std::vector PythonDependencyInstaller::getRequirementsFilePaths() const { + if (!std::filesystem::exists(python_processor_dir_)) { +return {}; + } + std::vector paths; + for (const auto& entry : std::filesystem::recursive_directory_iterator(std::filesystem::path{python_processor_dir_})) { +if (std::filesystem::is_regular_file(entry.path()) && entry.path().filename() == "requirements.txt") { + paths.push_back(entry.path()); +} + } + return paths; +} + +void PythonDependencyInstaller::createVirtualEnvIfSpecified() const { + if (virtualenv_path_.empty()) { Review Comment: Sounds good, added warning in 33e8770038e823678763c9bef60cc656c07ba16b -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559538680 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; Review Comment: Good catch, updated in 33e8770038e823678763c9bef60cc656c07ba16b ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + python_binary_ = getPythonBinary(configuration); + std::string automatic_install_str; + install_python_packages_automatically_ = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); Review Comment: Good idea, updated in 33e8770038e823678763c9bef60cc656c07ba16b -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559466021 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + python_binary_ = getPythonBinary(configuration); + std::string automatic_install_str; + install_python_packages_automatically_ = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); + if (auto path = configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) { +virtualenv_path_ = *path; +logger_->log_debug("Python virtualenv path was specified at: {}", virtualenv_path_.string()); + } else { +logger_->log_debug("No valid python virtualenv path was specified"); + } + if (auto python_processor_dir = configuration->get(minifi::Configuration::nifi_python_processor_dir)) { +python_processor_dir_ = *python_processor_dir; +logger_->log_debug("Python processor dir was specified at: {}", python_processor_dir_.string()); + } else { +logger_->log_debug("No valid python processor dir was not specified in properties"); + } + createVirtualEnvIfSpecified(); + addVirtualenvToPath(); +} + +std::vector PythonDependencyInstaller::getRequirementsFilePaths() const { + if (!std::filesystem::exists(python_processor_dir_)) { +return {}; + } + std::vector paths; + for (const auto& entry : std::filesystem::recursive_directory_iterator(std::filesystem::path{python_processor_dir_})) { +if (std::filesystem::is_regular_file(entry.path()) && entry.path().filename() == "requirements.txt") { + paths.push_back(entry.path()); +} + } + return paths; +} + +void PythonDependencyInstaller::createVirtualEnvIfSpecified() const { + if (virtualenv_path_.empty()) { Review Comment: should we log a warning if the virtualenv_path is empty but the automatic install is requested? -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559462716 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + python_binary_ = getPythonBinary(configuration); + std::string automatic_install_str; + install_python_packages_automatically_ = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); Review Comment: we could use the monadic functions here -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
adamdebreceni commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559445576 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,166 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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_; Review Comment: accidental trailing underscore? -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1559104966 ## libminifi/src/Configuration.cpp: ## @@ -152,7 +152,11 @@ const std::unordered_map
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1553978828 ## libminifi/src/Configuration.cpp: ## @@ -152,7 +152,11 @@ const std::unordered_map
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1539722380 ## extensions/python/PythonDependencyInstaller.h: ## @@ -0,0 +1,45 @@ +/** + * + * 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 +#include + +#include "PythonConfigState.h" +#include "core/logging/Logger.h" +#include "core/logging/LoggerConfiguration.h" +#include "properties/Configure.h" + +namespace org::apache::nifi::minifi::extensions::python { + +class PythonDependencyInstaller { + public: + explicit PythonDependencyInstaller(const std::shared_ptr &configuration); + void installDependenciesFromRequirementsFiles() const; + + private: + std::vector getRequirementsFilePaths() const; + void createVirtualEnvIfSpecified() const; + static void evalScript(std::string_view script); + void addVirtualenvToPath() const; + + PythonConfigState config_state_; Review Comment: Updated in 99359a0eda994fd5efa3095ebb76811ff1136f05 ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,167 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + config_state_.python_binary = getPythonBinary(configuration); + std::string automatic_install_str; + config_state_.install_python_packages_automatically = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); + if (auto path = configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) { +config_state_.virtualenv_path = *path; +logger_->log_debug("Python virtualenv path was specified at: {}", config_state_.virtualenv_path.string()); + } else { +logger_->log_debug("No valid python virtualenv path was specified"); + } + if (auto python_processor_dir = configuration->get(minifi::Configuration::nifi_python_processor_dir)) { +config_state_.python_processor_dir = *python_processor_dir; +logger_->log_debug("Python processor dir was specified at: {}", config_state_.python_processor_dir.string()); + } else { +logger_->log_debug("No valid python processor dir was not specified in properties"); + } + createVirtualEnvIfSpecified(); + addVirtualenvToPath(); +} + +std::vector PythonDependencyInstaller
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1539723152 ## extensions/python/PythonScriptEngine.h: ## @@ -84,7 +48,7 @@ class PythonScriptEngine { PythonScriptEngine& operator=(const PythonScriptEngine& other) = delete; PythonScriptEngine& operator=(PythonScriptEngine&& other) = delete; - static void initialize() {} + static void initialize(const std::shared_ptr &configuration, const std::shared_ptr& logger); Review Comment: Good catch, it was removed, but the declaration remained, removed in 99359a0eda994fd5efa3095ebb76811ff1136f05 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1532242813 ## extensions/python/PythonDependencyInstaller.h: ## @@ -0,0 +1,45 @@ +/** + * + * 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 +#include + +#include "PythonConfigState.h" +#include "core/logging/Logger.h" +#include "core/logging/LoggerConfiguration.h" +#include "properties/Configure.h" + +namespace org::apache::nifi::minifi::extensions::python { + +class PythonDependencyInstaller { + public: + explicit PythonDependencyInstaller(const std::shared_ptr &configuration); + void installDependenciesFromRequirementsFiles() const; + + private: + std::vector getRequirementsFilePaths() const; + void createVirtualEnvIfSpecified() const; + static void evalScript(std::string_view script); + void addVirtualenvToPath() const; + + PythonConfigState config_state_; Review Comment: It may be worth moving the data from PythonConfigState to this class, for more simplicity. It doesn't really add a new responsibility, just simplifies the existing one. ## extensions/python/PythonDependencyInstaller.cpp: ## @@ -0,0 +1,167 @@ +/** + * + * 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. + */ +#include "PythonDependencyInstaller.h" + +#include "PythonScriptException.h" +#include "PythonInterpreter.h" +#include "PyException.h" +#include "types/Types.h" + +namespace org::apache::nifi::minifi::extensions::python { + +namespace { + +std::string getPythonBinary(const std::shared_ptr &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; +} + +// On Windows when calling a system command using std::system, the whole command needs to be encapsulated in additional quotes, +// due to the std::system passing the command to 'cmd.exe /C' which needs the additional quotes to handle the command as a single argument +std::string encapsulateCommandInQuotesIfNeeded(const std::string& command) { +#if WIN32 +return "\"" + command + "\""; +#else +return command; +#endif +} + +} // namespace + +PythonDependencyInstaller::PythonDependencyInstaller(const std::shared_ptr &configuration) { + config_state_.python_binary = getPythonBinary(configuration); + std::string automatic_install_str; + config_state_.install_python_packages_automatically = + configuration->get(Configuration::nifi_python_install_packages_automatically, automatic_install_str) && utils::string::toBool(automatic_install_str).value_or(false); + if (auto path = configuration->get(minifi::Configuration::nifi_python_virtualenv_directory)) { +config_state_.virtualenv_path = *path; +logger_->log_debug("Python virtualenv path was specified at: {}", config_state_.virtualenv_path.string()); + } else { +logger_->log_debug("No valid python virtualenv path was specified"); + } + if (auto python_processor_dir = configuration->get(minifi::Configuration::nifi_python_processor_dir)) { +config_state_.python_processor_dir = *python_processor_dir; +logger_->log_debug("Python processor dir was specified at: {}", config_state_.python_processor_dir.string()); + } else { +logger_->log_debug("No valid python processor dir was not specified in propert
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1522863070 ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 +#include + +namespace org::apache::nifi::minifi::extensions::python { + +struct PythonConfigState { + public: + PythonConfigState(PythonConfigState&&) = delete; + PythonConfigState(const PythonConfigState&) = delete; + PythonConfigState& operator=(PythonConfigState&&) = delete; + PythonConfigState& operator=(const PythonConfigState&) = delete; + + bool isPackageInstallationNeeded() const { +return install_python_packages_automatically && !virtualenv_path.empty(); + } + + static PythonConfigState& getInstance() { +static PythonConfigState config; +return config; + } Review Comment: Extracted dependency installation to the `PythonDependencyInstaller` class and moved the dependency install to `PythonCreator`, to install dependencies while processing all the python processor script files in 5b20de2ffdc4d8f2bfd8907d1a47294440a11fc2 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1522853483 ## extensions/python/ExecutePythonProcessor.h: ## @@ -35,10 +35,6 @@ #include "PythonScriptEngine.h" #include "PythonScriptEngine.h" -#if defined(__GNUC__) || defined(__GNUG__) -#pragma GCC visibility push(hidden) Review Comment: This was added due to pybind11 requiring -fvisibility=hidden, but as we do not use pybind11 anymore this can be removed -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1519830502 ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 +#include + +namespace org::apache::nifi::minifi::extensions::python { + +struct PythonConfigState { + public: + PythonConfigState(PythonConfigState&&) = delete; + PythonConfigState(const PythonConfigState&) = delete; + PythonConfigState& operator=(PythonConfigState&&) = delete; + PythonConfigState& operator=(const PythonConfigState&) = delete; + + bool isPackageInstallationNeeded() const { +return install_python_packages_automatically && !virtualenv_path.empty(); + } + + static PythonConfigState& getInstance() { +static PythonConfigState config; +return config; + } Review Comment: I think it could be solved by returning or moving the list of dependencies from the python processor to the execution environment class, which can install them as needed. A callback could probably work. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1519685441 ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 +#include + +namespace org::apache::nifi::minifi::extensions::python { + +struct PythonConfigState { + public: + PythonConfigState(PythonConfigState&&) = delete; + PythonConfigState(const PythonConfigState&) = delete; + PythonConfigState& operator=(PythonConfigState&&) = delete; + PythonConfigState& operator=(const PythonConfigState&) = delete; + + bool isPackageInstallationNeeded() const { +return install_python_packages_automatically && !virtualenv_path.empty(); + } + + static PythonConfigState& getInstance() { +static PythonConfigState config; +return config; + } Review Comment: It makes sense in the context of this PR, I'm not sure what was the original intention of the `PythonScriptEngine::initialize` static function, as it was previously called with and empty bode from the `PythonCreator`, so my thought was that it was supposed to implement the global python environment specific initialization, but it can be moved to the `PythonCreator` or a separate class. The problem is that in the PR depending on this https://github.com/apache/nifi-minifi-cpp/pull/1727 we introduce the support for inline python dependency installation, which is separately done for every NiFi python processor initialization that is called from the `ExecutePythonProcessor`. We also only want to initialize the processors that are part of the minifi flow config in the future. The inline python dependency installation needs the information stored in the `PythonConfigState` that is why it needs to be reached from outside the `PythonCreator` -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1519636902 ## 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 getRequirementsFilePaths(const std::shared_ptr &configuration) { + std::vector 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 &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 &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 &configuration, const std::shared_ptr& 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: Added comment in c8e483279209a6d6a843e0f343876ff9adb97c93 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1519487010 ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 +#include + +namespace org::apache::nifi::minifi::extensions::python { + +struct PythonConfigState { + public: + PythonConfigState(PythonConfigState&&) = delete; + PythonConfigState(const PythonConfigState&) = delete; + PythonConfigState& operator=(PythonConfigState&&) = delete; + PythonConfigState& operator=(const PythonConfigState&) = delete; + + bool isPackageInstallationNeeded() const { +return install_python_packages_automatically && !virtualenv_path.empty(); + } + + static PythonConfigState& getInstance() { +static PythonConfigState config; +return config; + } Review Comment: Since PythonScriptEngine is created on a per-processor basis, we can't let it handle the creation of the shared execution environment. That should be extracted to a separate class, which could be owned by e.g. PythonCreator, setting up the virtualenv and packages, so that individual PythonScriptEngines can be created in that environment. PythonConfigState could be part of this new class, and PythonCreator could inject the Configuration object into it. What do you think about this? -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1517807712 ## encrypt-config/tests/ConfigFileEncryptorTests.cpp: ## @@ -77,7 +77,7 @@ TEST_CASE("ConfigFileEncryptor can encrypt the sensitive properties", "[encrypt- uint32_t num_properties_encrypted = encryptSensitivePropertiesInFile(test_file, KEY); REQUIRE(num_properties_encrypted == 1); -REQUIRE(test_file.size() == 110); +REQUIRE(test_file.size() == 115); Review Comment: When the default minifi.properties file changes this test always needs to be updated, as it checks the property count. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1517807712 ## encrypt-config/tests/ConfigFileEncryptorTests.cpp: ## @@ -77,7 +77,7 @@ TEST_CASE("ConfigFileEncryptor can encrypt the sensitive properties", "[encrypt- uint32_t num_properties_encrypted = encryptSensitivePropertiesInFile(test_file, KEY); REQUIRE(num_properties_encrypted == 1); -REQUIRE(test_file.size() == 110); +REQUIRE(test_file.size() == 115); Review Comment: When the default minifi.properties file changes this test always needs to be updated, as it checks the line count. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1517821922 ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 +#include + +namespace org::apache::nifi::minifi::extensions::python { + +struct PythonConfigState { + public: + PythonConfigState(PythonConfigState&&) = delete; + PythonConfigState(const PythonConfigState&) = delete; + PythonConfigState& operator=(PythonConfigState&&) = delete; + PythonConfigState& operator=(const PythonConfigState&) = delete; + + bool isPackageInstallationNeeded() const { +return install_python_packages_automatically && !virtualenv_path.empty(); + } + + static PythonConfigState& getInstance() { +static PythonConfigState config; +return config; + } Review Comment: You are right, I also try to avoid them when possible. In this case the python configuration is read in the PythonCreator from the Configuration object, but has to be used in the PythonScriptEngine which is initialized in each python processor. Due to the interface we use to create the python processors (instantiating the ExecutePythonProcessor class from the PythonObjectFactory) there is no clean way currently to reach the `Configuration` object from the processors' scope. Even though it's better to avoid them I think this is still a cleaner solution than anything else I could come up with. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1517807712 ## encrypt-config/tests/ConfigFileEncryptorTests.cpp: ## @@ -77,7 +77,7 @@ TEST_CASE("ConfigFileEncryptor can encrypt the sensitive properties", "[encrypt- uint32_t num_properties_encrypted = encryptSensitivePropertiesInFile(test_file, KEY); REQUIRE(num_properties_encrypted == 1); -REQUIRE(test_file.size() == 110); +REQUIRE(test_file.size() == 115); Review Comment: When the default minifi.properties file changes this tests always needs to be updated, as it checks the line count. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1517806663 ## 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: This is just for using fixed version of the library, to avoid breaking the tests with newer versions, see comment https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1513218719 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
szaszm commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1511271227 ## 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: What happened in langchain >0.17? ## 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 getRequirementsFilePaths(const std::shared_ptr &configuration) { + std::vector 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 &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 &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 &configuration, const std::shared_ptr& 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: Could you add this explanation into a code comment, maybe above the quoting function? I had the very same question when reading the code. ## encrypt-config/tests/ConfigFileEncryptorTests.cpp: ## @@ -77,7 +77,7 @@ TEST_CASE("ConfigFileEncryptor can encrypt the sensitive properties", "[encrypt- uint32_t num_properties_encrypted = encryptSensitivePropertiesInFile(test_file, KEY); REQUIRE(num_properties_encrypted == 1); -REQUIRE(test_file.size() == 110); +REQUIRE(test_file.size() == 115); Review Comment: what's the increase? ## extensions/python/PythonConfigState.h: ## @@ -0,0 +1,50 @@ +/** + * + * 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 regardin
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1514070824 ## 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 getRequirementsFilePaths(const std::shared_ptr &configuration) { + std::vector 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 &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 &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: Good catch, we should only create a virtualenv if the directory is empty, probably didn't update the condition while refactoring and didn't retest this, updated in 44654b11b315ed6010424cb72abb334b8b286377 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1514069231 ## 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 +#include +#include +#include +#include +#include +#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: Good point, left them in by mistake, removed the unneeded ones in 44654b11b315ed6010424cb72abb334b8b286377 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1514068298 ## 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: You are right it's more clear this way, updated in 44654b11b315ed6010424cb72abb334b8b286377 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1514067762 ## 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: They are unused, removed in 44654b11b315ed6010424cb72abb334b8b286377 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1513224888 ## 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 getRequirementsFilePaths(const std::shared_ptr &configuration) { + std::vector 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 &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 &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 &configuration, const std::shared_ptr& 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: Yes it looks strange, but there is an issue with std::system on Windows that needs the additional quotes on the whole command to work correctly. If I remember well it passes the system command as a parameter to the `cmd.exe /C` command and the additional quotes are needed to parse the system command's arguments correctly. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
lordgamez commented on code in PR #1721: URL: https://github.com/apache/nifi-minifi-cpp/pull/1721#discussion_r1513218719 ## 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: Currently the latest version of langchain is 0.17.0 and as we run the python tests on multiple distributions and with virtualenvs or with conda in the CI, we run these tests with version 0.17.0 or bwloe. The tests with these versions are tested and passing, but the langchain library is rapidly changing so we don't want to use the latest version in the tests because they may break with a future version, so it is better to have the version fixed. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2277 Add virtualenv support for python processors [nifi-minifi-cpp]
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 getRequirementsFilePaths(const std::shared_ptr &configuration) { + std::vector paths; + if (auto python_processor_path = configuration->get(minifi::Configuration::nifi_python_processor_dir)) { +for (const auto& entry : std::fil