[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1504: MINIFICPP-1961 Python scripting compatibility with multiple python minor versions

["fgerlits (via GitHub)" <gi...@apache.org>]

fgerlits commented on code in PR #1504:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1504#discussion_r1121850761


##########
docker/DockerVerify.sh:
##########
@@ -17,17 +17,114 @@
 
 set -e
 
-if [[ $# -lt 2 ]]; then
-  echo "Usage:"
-  echo "  ./DockerVerify.sh <MINIFI_VERSION> <FEATURE_PATH>"
-  exit 1
-fi
+die()
+{
+  local _ret="${2:-1}"
+  test "${_PRINT_HELP:-no}" = yes && print_help >&2
+  echo "$1" >&2
+  exit "${_ret}"
+}
+
+
+begins_with_short_option()
+{
+  local first_option all_short_options='h'
+  first_option="${1:0:1}"
+  test "$all_short_options" = "${all_short_options/$first_option/}" && return 1 || return 0
+}

Review Comment:
   where is this used?



##########
extensions/lua/LuaScriptEngine.cpp:
##########
@@ -0,0 +1,137 @@
+/**
+ * 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 <memory>
+#include <string>
+#include <filesystem>
+
+#include "LuaScriptEngine.h"
+#include "LuaProcessSession.h"
+#include "LuaScriptProcessContext.h"
+#include "utils/StringUtils.h"
+
+namespace org::apache::nifi::minifi::extensions::lua {
+
+
+LuaScriptEngine::LuaScriptEngine() {
+  lua_.open_libraries(sol::lib::base,
+      sol::lib::os,
+      sol::lib::coroutine,
+      sol::lib::math,
+      sol::lib::io,
+      sol::lib::string,
+      sol::lib::table,
+      sol::lib::utf8,
+      sol::lib::package);
+  lua_.new_usertype<core::logging::Logger>(
+      "Logger",
+      "info", &core::logging::Logger::log_info<>);
+  lua_.new_usertype<LuaProcessSession>(
+      "ProcessSession",
+      "create", static_cast<std::shared_ptr<LuaScriptFlowFile> (LuaProcessSession::*)()>(&LuaProcessSession::create),
+      "get", &LuaProcessSession::get,
+      "read", &LuaProcessSession::read,
+      "write", &LuaProcessSession::write,
+      "transfer", &LuaProcessSession::transfer,
+      "remove", &LuaProcessSession::remove);
+  lua_.new_usertype<LuaScriptFlowFile>(
+      "FlowFile",
+      "getAttribute", &LuaScriptFlowFile::getAttribute,
+      "addAttribute", &LuaScriptFlowFile::addAttribute,
+      "removeAttribute", &LuaScriptFlowFile::removeAttribute,
+      "updateAttribute", &LuaScriptFlowFile::updateAttribute,
+      "setAttribute", &LuaScriptFlowFile::setAttribute);
+  lua_.new_usertype<LuaInputStream>(
+      "InputStream",
+      "read", &LuaInputStream::read);
+  lua_.new_usertype<LuaOutputStream>(
+      "OutputStream",
+      "write", &LuaOutputStream::write);
+}
+
+void LuaScriptEngine::executeScriptWithAppendedModulePaths(std::string& script) {
+  for (const auto& module_path : module_paths_) {
+    if (std::filesystem::is_regular_file(std::filesystem::status(module_path))) {
+      script = utils::StringUtils::join_pack("package.path = package.path .. [[;", module_path.string(), "]]\n", script);
+    } else {
+      script = utils::StringUtils::join_pack("package.path = package.path .. [[;", module_path.string(), "/?.lua]]\n", script);
+    }
+  }
+  lua_.script(script, sol::script_throw_on_error);
+}
+
+void LuaScriptEngine::eval(const std::string& script) {
+  try {
+    if (!module_paths_.empty()) {
+      auto appended_script = script;
+      executeScriptWithAppendedModulePaths(appended_script);
+    } else {
+      lua_.script(script, sol::script_throw_on_error);
+    }
+  } catch (std::exception& e) {
+    throw LuaScriptException(e.what());
+  }
+}
+
+void LuaScriptEngine::evalFile(const std::filesystem::path& file_name) {
+  try {
+    if (!module_paths_.empty()) {
+      std::ifstream stream(file_name);
+      std::string script((std::istreambuf_iterator<char>(stream)), std::istreambuf_iterator<char>());
+      executeScriptWithAppendedModulePaths(script);
+    } else {
+      lua_.script_file(file_name.string(), sol::script_throw_on_error);
+    }
+  } catch (std::exception& e) {
+    throw LuaScriptException(e.what());
+  }
+}
+
+void LuaScriptEngine::initialize(const core::Relationship& success, const core::Relationship& failure, const std::shared_ptr<core::logging::Logger>& logger) {
+  bind("log", logger);
+  bind("REL_SUCCESS", success);
+  bind("REL_FAILURE", failure);
+}
+
+namespace {
+class TriggerSession {
+ public:
+  TriggerSession(std::shared_ptr<LuaScriptProcessContext> script_context,
+      std::shared_ptr<LuaProcessSession> lua_session)
+      : script_context_(std::move(script_context)),
+      lua_session_(std::move(lua_session)) {
+  }
+
+  ~TriggerSession() {
+    script_context_->releaseProcessContext();
+    lua_session_->releaseCoreResources();
+  }
+
+ private:
+  std::shared_ptr<LuaScriptProcessContext> script_context_;
+  std::shared_ptr<LuaProcessSession> lua_session_;
+};
+}  // namespace
+
+void LuaScriptEngine::onTrigger(const std::shared_ptr<core::ProcessContext>& context, const std::shared_ptr<core::ProcessSession>& session) {
+auto script_context = convert(context);
+auto lua_session = convert(session);
+TriggerSession trigger_session(script_context, lua_session);
+call("onTrigger", script_context, lua_session);

Review Comment:
   missing indentation



##########
extensions/lua/LuaScriptExecutor.cpp:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 <string>
+#include <filesystem>
+
+#include "LuaScriptExecutor.h"
+#include "range/v3/range/conversion.hpp"
+#include "Resource.h"
+
+namespace org::apache::nifi::minifi::extensions::lua {
+
+LuaScriptExecutor::LuaScriptExecutor(std::string name, const utils::Identifier& uuid) : script::ScriptExecutor(std::move(name), uuid) {}
+
+void LuaScriptExecutor::onTrigger(const std::shared_ptr<core::ProcessContext>& context, const std::shared_ptr<core::ProcessSession>& session) {
+  auto lua_script_engine = lua_script_engine_queue_->getResource();
+  gsl_Expects(std::holds_alternative<std::filesystem::path>(script_to_run_) || std::holds_alternative<std::string>(script_to_run_));
+
+  if (module_directory_) {
+    lua_script_engine->setModulePaths(utils::StringUtils::splitAndTrimRemovingEmpty(*module_directory_, ",") | ranges::to<std::vector<std::filesystem::path>>());
+  }
+
+  if (std::holds_alternative<std::filesystem::path>(script_to_run_))
+    lua_script_engine->evalFile(std::get<std::filesystem::path>(script_to_run_));
+  else
+    lua_script_engine->eval(std::get<std::string>(script_to_run_));
+
+  lua_script_engine->onTrigger(context, session);
+}
+
+void LuaScriptExecutor::initialize(std::filesystem::path script_file,
+    std::string script_body,
+    std::optional<std::string> module_directory,
+    size_t max_concurrent_engines,
+    const core::Relationship& success,
+    const core::Relationship& failure,
+    std::shared_ptr<core::logging::Logger> logger) {
+  if (script_file.empty() == script_body.empty())
+    throw std::runtime_error("Exactly one of these must be non-zero: ScriptBody, ScriptFile");

Review Comment:
   nitpicking, but
   ```suggestion
       throw std::runtime_error("Exactly one of these must be non-empty: ScriptBody, ScriptFile");
   ```



##########
bstrp_functions.sh:
##########
@@ -371,7 +371,7 @@ show_supported_features() {
   echo "J. TensorFlow Support ..........$(print_feature_status TENSORFLOW_ENABLED)"
   echo "K. Bustache Support ............$(print_feature_status BUSTACHE_ENABLED)"
   echo "L. MQTT Support ................$(print_feature_status MQTT_ENABLED)"
-  echo "M. Python Support ..............$(print_feature_status PYTHON_ENABLED)"
+  echo "M. Lua Scripting Support .......$(print_feature_status LUA_SCRIPTING_ENABLED)"

Review Comment:
   I know we can't make all the letters fit the extension, but could we switch these two, please? L -> Lua, M -> MQTT



##########
docker/test/integration/features/python.feature:
##########
@@ -54,3 +54,11 @@ Feature: MiNiFi can use python processors in its flows
     When all instances start up
     Then the Minifi logs contain the following message: "Removing flow file with UUID" in less than 30 seconds
 
+  Scenario: Native python processors can be stateful
+    Given a CountingProcessor processor
+    And the scheduling period of the CountingProcessor processor is set to "1 sec"

Review Comment:
   can this be shorter? e.g., 100 ms?



##########
extensions/python/pythonprocessors/examples/GaussianDistributionWithNumpy.py:
##########
@@ -0,0 +1,42 @@
+# 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.
+
+import numpy as np
+
+
+class WriteReverseStringCallback:
+    def __init__(self, content):
+        self.content = content
+
+    def process(self, output_stream):
+        output_stream.write(self.content.encode('utf-8'))
+        return len(self.content)

Review Comment:
   This doesn't reverse the output.  I would rename it `WriteCallback`.  (Or we could add reversing, but why would this processor want to reverse its output?)



##########
.github/workflows/verify-python-compatibility.yml:
##########
@@ -0,0 +1,140 @@
+name: "MiNiFi-CPP Verify Python Compatibility"
+on: [workflow_dispatch]
+jobs:
+  centos-build:
+    name: "Build centos"
+    runs-on: ubuntu-22.04
+    timeout-minutes: 180
+    steps:
+      - id: checkout
+        uses: actions/checkout@v3
+      - id: cache
+        uses: actions/cache@v3
+        with:
+          path: ~/.ccache
+          key: centos-build-with-python-ccache-${{github.ref}}-${{github.sha}}
+          restore-keys: |
+            centos-build-with-python-ccache-${{github.ref}}-
+            centos-build-with-python-ccache-refs/heads/main-
+      - id: install_deps
+        run: |
+          sudo apt update
+          sudo apt install -y ccache cmake
+          echo "PATH=/usr/lib/ccache:$PATH" >> $GITHUB_ENV
+      - id: build
+        run: |
+          if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
+          mkdir build && cd build && cmake -DENABLE_PYTHON_SCRIPTING=ON -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache .. && make centos
+      - uses: actions/upload-artifact@v3
+        with:
+          name: minifi-tar
+          path: build/nifi-minifi-cpp-*-bin-centos.tar.gz
+          if-no-files-found: error
+  rocky-linux8:
+    name: "Python tests on Rocky Linux 8"
+    runs-on: ubuntu-20.04

Review Comment:
   why are we running these on ubuntu-20.04 and not on ubuntu-22.04?



-- 
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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org