You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "fgerlits (via GitHub)" <gi...@apache.org> on 2024/10/21 15:56:07 UTC
[PR] NIFI-13824 Install processor dependencies in one go [nifi]
fgerlits opened a new pull request, #9429:
URL: https://github.com/apache/nifi/pull/9429
<!-- 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. -->
# Summary
[NIFI-13824](https://issues.apache.org/jira/browse/NIFI-13824)
If a Python processor defines dependencies both inline and in a requirements.txt file, then we need to install the two groups of dependencies in a single `pip install` command, otherwise `pip` is not able to resolve the web of dependencies correctly.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `./mvnw clean install -P contrib-check`
- [ ] JDK 21
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1810517927
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/resources/python/framework/processor_with_dependencies/requirements.txt:
##########
@@ -0,0 +1 @@
+google-cloud-vision==3.7.4
Review Comment:
I have added the header in 6d97284da4104a0695a86c23ecd80aa5ac104629
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1810515104
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,90 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
+ processor_packages_dir = os.path.join(packages_dir, 'extensions', 'ProcessorWithDependencies', '0.0.1')
+ pip_list_result = subprocess.run([python_command, '-m', 'pip', 'list', '--path', processor_packages_dir], capture_output=True)
+ self.assertEqual(pip_list_result.returncode, 0)
+ pip_list_lines = [line.decode() for line in pip_list_result.stdout.splitlines()]
+
+ grpcio_version = get_version(pip_list_lines, 'grpcio')
+ self.assertIsNotNone(grpcio_version)
+ grpcio_status_version = get_version(pip_list_lines, 'grpcio-status')
+ self.assertEqual(grpcio_version, grpcio_status_version)
Review Comment:
I have added the comments in bc87377435b02c67a599d8bdc6a0868831148639
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1816515662
##########
.github/workflows/ci-workflow.yml:
##########
@@ -162,6 +162,10 @@ jobs:
distribution: 'corretto'
java-version: '21'
cache: 'maven'
+ - name: Set up Python 3.12
+ uses: actions/setup-python@v5
+ with:
+ python-version: '3.12'
Review Comment:
This is a bugfix: we were already using Python in unit tests, and without this step, the CI jobs were using whatever Python version was available by default in the GitHub Action image (which happened to be a homebrew-installed Python 3.13 in the case of the MacOS image). With the explicit setup step, now the CI jobs are using Python 3.12 in a predictable and reproducible way.
Also, with system tests using Python 3.10 and unit tests using Python 3.12, we can verify that the Python components in NiFi work with both, which is a good thing.
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1809115473
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/main/python/framework/ExtensionManager.py:
##########
@@ -285,37 +285,31 @@ def import_external_dependencies(self, processor_details, work_dir):
logger.info("All dependencies have already been imported for {0}".format(class_name))
return True
- python_cmd = os.getenv("PYTHON_CMD")
+ packages_to_install = []
Review Comment:
Recommend renaming this variable as it includes both packages and `requirements.txt` when found.
```suggestion
dependency_references = []
```
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,90 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
+ processor_packages_dir = os.path.join(packages_dir, 'extensions', 'ProcessorWithDependencies', '0.0.1')
+ pip_list_result = subprocess.run([python_command, '-m', 'pip', 'list', '--path', processor_packages_dir], capture_output=True)
+ self.assertEqual(pip_list_result.returncode, 0)
+ pip_list_lines = [line.decode() for line in pip_list_result.stdout.splitlines()]
+
+ grpcio_version = get_version(pip_list_lines, 'grpcio')
+ self.assertIsNotNone(grpcio_version)
+ grpcio_status_version = get_version(pip_list_lines, 'grpcio-status')
+ self.assertEqual(grpcio_version, grpcio_status_version)
Review Comment:
With the assertion based on the `grpcio` library, I recommend changing the dependencies to list it explicitly, instead of depending on it as a transitive dependency.
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/resources/python/framework/processor_with_dependencies/requirements.txt:
##########
@@ -0,0 +1 @@
+google-cloud-vision==3.7.4
Review Comment:
This file is missing the license header.
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1810514497
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/main/python/framework/ExtensionManager.py:
##########
@@ -285,37 +285,31 @@ def import_external_dependencies(self, processor_details, work_dir):
logger.info("All dependencies have already been imported for {0}".format(class_name))
return True
- python_cmd = os.getenv("PYTHON_CMD")
+ packages_to_install = []
Review Comment:
I have renamed it in c32d56bfb247e80fb072c062ea6b1e32d1a4f3d2
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #9429: NIFI-13824 Install processor dependencies in one go
URL: https://github.com/apache/nifi/pull/9429
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1811095650
##########
mvnw:
##########
@@ -105,6 +106,21 @@ trim() {
printf "%s" "${1}" | tr -d '[:space:]'
}
+# find the Python executable
+if [ -z "${PYTHON_CMD-}" ]; then
+ for PYTHON_PATH in /usr/bin/python3 /usr/local/bin/python3 /usr/bin/python /usr/local/bin/python; do
+ if [ -x "${PYTHON_PATH}" ]; then
+ export PYTHON_CMD="${PYTHON_PATH}"
+ break
+ fi
+ done
+
+ if [ -z "${PYTHON_CMD-}" ]; then
+ echo "Python executable not found; please set PYTHON_CMD" >&2
+ return 1
+ fi
+fi
Review Comment:
fixed in 7af4f4ce001d5f3c6fb22b208c10535d109d3fc3
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1810515104
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,90 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
+ processor_packages_dir = os.path.join(packages_dir, 'extensions', 'ProcessorWithDependencies', '0.0.1')
+ pip_list_result = subprocess.run([python_command, '-m', 'pip', 'list', '--path', processor_packages_dir], capture_output=True)
+ self.assertEqual(pip_list_result.returncode, 0)
+ pip_list_lines = [line.decode() for line in pip_list_result.stdout.splitlines()]
+
+ grpcio_version = get_version(pip_list_lines, 'grpcio')
+ self.assertIsNotNone(grpcio_version)
+ grpcio_status_version = get_version(pip_list_lines, 'grpcio-status')
+ self.assertEqual(grpcio_version, grpcio_status_version)
Review Comment:
I have add the comments in bc87377435b02c67a599d8bdc6a0868831148639
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1811116947
##########
mvnw:
##########
@@ -105,6 +106,21 @@ trim() {
printf "%s" "${1}" | tr -d '[:space:]'
}
+# find the Python executable
+if [ -z "${PYTHON_CMD-}" ]; then
+ for PYTHON_PATH in /usr/bin/python3 /usr/local/bin/python3 /usr/bin/python /usr/local/bin/python; do
+ if [ -x "${PYTHON_PATH}" ]; then
+ export PYTHON_CMD="${PYTHON_PATH}"
+ break
+ fi
+ done
+
+ if [ -z "${PYTHON_CMD-}" ]; then
+ echo "Python executable not found; please set PYTHON_CMD" >&2
+ return 1
+ fi
+fi
Review Comment:
Thanks for the adjusted approach @fgerlits.
Reviewing the test itself, it seems like this is more suitable as an integration test under the `nifi-system-test-suite` module. The `system-tests` GitHub workflow already installs Python, and it is a better place to run longer types of tests, such as this one that require installing dependency sets. What do you think about refactoring this test into the system test module?
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1810701611
##########
mvnw:
##########
@@ -105,6 +106,21 @@ trim() {
printf "%s" "${1}" | tr -d '[:space:]'
}
+# find the Python executable
+if [ -z "${PYTHON_CMD-}" ]; then
+ for PYTHON_PATH in /usr/bin/python3 /usr/local/bin/python3 /usr/bin/python /usr/local/bin/python; do
+ if [ -x "${PYTHON_PATH}" ]; then
+ export PYTHON_CMD="${PYTHON_PATH}"
+ break
+ fi
+ done
+
+ if [ -z "${PYTHON_CMD-}" ]; then
+ echo "Python executable not found; please set PYTHON_CMD" >&2
+ return 1
+ fi
+fi
Review Comment:
The Maven wrapper script comes from the Apache Maven project and should not be modified.
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,94 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
Review Comment:
Using `sys.executable` here may be a more reliable option.
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1816933373
##########
.github/workflows/ci-workflow.yml:
##########
@@ -162,6 +162,10 @@ jobs:
distribution: 'corretto'
java-version: '21'
cache: 'maven'
+ - name: Set up Python 3.12
+ uses: actions/setup-python@v5
+ with:
+ python-version: '3.12'
Review Comment:
Thanks for clarifying, that makes sense, and I agree that installing a specific version is the right approach.
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1809261537
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,90 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
+ processor_packages_dir = os.path.join(packages_dir, 'extensions', 'ProcessorWithDependencies', '0.0.1')
+ pip_list_result = subprocess.run([python_command, '-m', 'pip', 'list', '--path', processor_packages_dir], capture_output=True)
+ self.assertEqual(pip_list_result.returncode, 0)
+ pip_list_lines = [line.decode() for line in pip_list_result.stdout.splitlines()]
+
+ grpcio_version = get_version(pip_list_lines, 'grpcio')
+ self.assertIsNotNone(grpcio_version)
+ grpcio_status_version = get_version(pip_list_lines, 'grpcio-status')
+ self.assertEqual(grpcio_version, grpcio_status_version)
Review Comment:
The point of this test is that both packages depend on `grpcio`, but with different version bounds, so if we install one after the other, we'll end up with inconsistent versions. If I add a specific `grpcio` version to both places, then the old code will work, as well, so the test will no longer show what was fixed.
Is it OK if I add a comment here in the test file with an explanation of why we are checking the `grpcio` versions?
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1814613566
##########
mvnw:
##########
@@ -105,6 +106,21 @@ trim() {
printf "%s" "${1}" | tr -d '[:space:]'
}
+# find the Python executable
+if [ -z "${PYTHON_CMD-}" ]; then
+ for PYTHON_PATH in /usr/bin/python3 /usr/local/bin/python3 /usr/bin/python /usr/local/bin/python; do
+ if [ -x "${PYTHON_PATH}" ]; then
+ export PYTHON_CMD="${PYTHON_PATH}"
+ break
+ fi
+ done
+
+ if [ -z "${PYTHON_CMD-}" ]; then
+ echo "Python executable not found; please set PYTHON_CMD" >&2
+ return 1
+ fi
+fi
Review Comment:
You are right, a unit test should not download stuff from the internet (and take 20+ seconds). In e3ab1c7c53742fe22cddcc8384018e3a55e93f81, I have mocked out the `subprocess.run` call, and now only test that it gets called once instead of twice, as it used to be.
I also agree it would be a good idea to add an integration test which does the package installations and version checks; can we do that separately, in another pull request?
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1809287343
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-py4j-framework-bundle/nifi-python-framework/src/test/python/framework/TestDownloadDependencies.py:
##########
@@ -0,0 +1,90 @@
+# 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 os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from ExtensionManager import ExtensionManager
+from nifiapi.__jvm__ import JvmHolder
+import ProcessorInspection
+
+TEST_PROCESSOR_FILE = 'src/test/resources/python/framework/processor_with_dependencies/ProcessorWithDependencies.py'
+
+class FakeJvm:
+ def __init__(self):
+ self.java = FakeJava()
+
+class FakeJava:
+ def __init__(self):
+ self.util = FakeJavaUtil()
+
+class FakeJavaUtil:
+ def ArrayList(self):
+ return FakeArrayList([])
+
+class FakeArrayList:
+ def __init__(self, my_list):
+ self.my_list = my_list
+
+ def __len__(self):
+ return len(self.my_list)
+
+ def __iter__(self):
+ return iter(self.my_list)
+
+ def add(self, element):
+ self.my_list.append(element)
+
+def get_version(pip_list_lines, package_name):
+ for line in pip_list_lines:
+ match = re.fullmatch(f"{package_name} +([0-9.]+)", line)
+ if match:
+ return match[1]
+ return None
+
+class TestDownloadDependencies(unittest.TestCase):
+ def test_import_external_dependencies(self):
+ class_nodes = ProcessorInspection.get_processor_class_nodes(TEST_PROCESSOR_FILE)
+ self.assertIsNotNone(class_nodes)
+ self.assertEqual(len(class_nodes), 1)
+ class_node = class_nodes[0]
+ self.assertEqual(class_node.name, 'ProcessorWithDependencies')
+
+ details = ProcessorInspection.get_processor_details(class_node, TEST_PROCESSOR_FILE, '/extensions/processor_with_dependencies', False)
+ self.assertIsNotNone(details)
+
+ JvmHolder.jvm = FakeJvm()
+ extension_manager = ExtensionManager(None)
+
+ with tempfile.TemporaryDirectory() as temp_dir:
+ packages_dir = os.path.join(temp_dir, 'packages')
+ extension_manager.import_external_dependencies(details, packages_dir)
+
+ python_command = os.environ.get("PYTHON_CMD")
+ processor_packages_dir = os.path.join(packages_dir, 'extensions', 'ProcessorWithDependencies', '0.0.1')
+ pip_list_result = subprocess.run([python_command, '-m', 'pip', 'list', '--path', processor_packages_dir], capture_output=True)
+ self.assertEqual(pip_list_result.returncode, 0)
+ pip_list_lines = [line.decode() for line in pip_list_result.stdout.splitlines()]
+
+ grpcio_version = get_version(pip_list_lines, 'grpcio')
+ self.assertIsNotNone(grpcio_version)
+ grpcio_status_version = get_version(pip_list_lines, 'grpcio-status')
+ self.assertEqual(grpcio_version, grpcio_status_version)
Review Comment:
Thanks for clarifying, that is helpful. Yes, I think simply adding a comment above these assertions is sufficient.
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on PR #9429:
URL: https://github.com/apache/nifi/pull/9429#issuecomment-2428981867
> the new Python unit test is failing on both macOS and Ubuntu Linux
This was because we need to set `PYTHON_CMD` for some of the Python code to work; fixed in 3889d12beb6162172ca5ee70c7ec9a47a9450210
--
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
Re: [PR] NIFI-13824 Install processor dependencies in one go [nifi]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #9429:
URL: https://github.com/apache/nifi/pull/9429#discussion_r1816518525
##########
mvnw:
##########
@@ -105,6 +106,21 @@ trim() {
printf "%s" "${1}" | tr -d '[:space:]'
}
+# find the Python executable
+if [ -z "${PYTHON_CMD-}" ]; then
+ for PYTHON_PATH in /usr/bin/python3 /usr/local/bin/python3 /usr/bin/python /usr/local/bin/python; do
+ if [ -x "${PYTHON_PATH}" ]; then
+ export PYTHON_CMD="${PYTHON_PATH}"
+ break
+ fi
+ done
+
+ if [ -z "${PYTHON_CMD-}" ]; then
+ echo "Python executable not found; please set PYTHON_CMD" >&2
+ return 1
+ fi
+fi
Review Comment:
This unit test has value: it loads the ExtensionManager class, so if there are e.g. syntax errors, they get caught earlier; and it validates the change in the PR in the sense that if you run the test with the ExtensionManager code on `main`, it will fail: we are now calling `pip install` once instead of twice.
I have renamed and refactored it to make it clearer that this is just a unit test for a single method in the ExtensionManager class, and also to make it easier to add unit tests for the rest of the methods later.
--
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