Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINIFICPP-1961 Python scripting compatibility with multiple python minor versions #1504

Closed
wants to merge 44 commits into from

Conversation

martinzink
Copy link
Member

The bulk of this python rewrite was done by @dam4rus. I've took over because he got other responsibilities that made it unfeasable to finish the PR.


This change replaces the previous Python extension implementation (based on PyBind11) with a pure Python C API implementation (with the limited stable API) https://docs.python.org/3/c-api/stable.html

I've included a new ci workflow that uses the centos tar.gz and verifies this new extension works on a large variety of platforms with large variety of python versions, but I don't think we want to run these on every commit. (Maybe on every release?)

I've also separated the current scripting extensions into lua/python extensions so it will be possible to build them together, and still use them without needing the other one's dependencies.


Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.

@martinzink martinzink marked this pull request as ready for review February 8, 2023 09:45
@martinzink
Copy link
Member Author

I've ran the new python compatibility tests on my fork.
https://github.com/martinzink/nifi-minifi-cpp/actions/runs/4114945925
After we merge this PR, we should be able to run this from github's UI

@martinzink
Copy link
Member Author

martinzink commented Feb 9, 2023

I've followed @szaszm advice and readded ExecuteScript in a3b6e6b, so this PR doesn't break compatibility with older configs, and follows NiFi's way of doing ExecuteScript more closely.
A ScriptExecutor is loaded through ClassLoader, this means to use ExecuteScript you will need the script-extension aswell as the python-script-extension/lua-script-extension to use python/lua scripting.

This of course intruduces a bit of additional complexity (and maybe restrictions) vs the separate ExecuteLuaScript/ExecutePythonScript approach. Let me know what you think.

PROCESSORS.md Outdated Show resolved Hide resolved
bootstrap.sh Show resolved Hide resolved
docker/DockerVerify.sh Show resolved Hide resolved
docker/python-verify/conda.Dockerfile Show resolved Hide resolved
docker/python-verify/conda.Dockerfile Outdated Show resolved Hide resolved
extensions/lua/tests/CMakeLists.txt Outdated Show resolved Hide resolved
extensions/python/PythonBindings.cpp Outdated Show resolved Hide resolved
extensions/python/PythonBindings.cpp Outdated Show resolved Hide resolved
extensions/python/PythonScriptFlowFile.h Outdated Show resolved Hide resolved
thirdparty/google-styleguide/cpplint.py Outdated Show resolved Hide resolved
win_build_vs.bat Show resolved Hide resolved
@martinzink
Copy link
Member Author

I've also reran the compatibility tests after these changes.

extensions/python/tests/CMakeLists.txt Outdated Show resolved Hide resolved
extensions/python/tests/CMakeLists.txt Show resolved Hide resolved
extensions/python/tests/PythonScriptEngineTests.cpp Outdated Show resolved Hide resolved
extensions/script/ExecuteScript.cpp Outdated Show resolved Hide resolved
extensions/python/types/PyProcessSession.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fgerlits fgerlits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't got to the end of this, but only minor quibbles so far.

bstrp_functions.sh Outdated Show resolved Hide resolved
docker/DockerVerify.sh Outdated Show resolved Hide resolved
docker/test/integration/features/python.feature Outdated Show resolved Hide resolved
extensions/lua/LuaScriptEngine.cpp Outdated Show resolved Hide resolved
extensions/lua/LuaScriptExecutor.cpp Outdated Show resolved Hide resolved
@szaszm szaszm closed this in 70015a3 Mar 7, 2023
lordgamez pushed a commit to lordgamez/nifi-minifi-cpp that referenced this pull request Mar 9, 2023
To support binaries targeting multiple libpython minor versions.

Closes apache#1504
Signed-off-by: Marton Szasz <szaszm@apache.org>
Co-authored-by: Martin Zink <martinzink@apache.org>
lordgamez pushed a commit to lordgamez/nifi-minifi-cpp that referenced this pull request Mar 9, 2023
To support binaries targeting multiple libpython minor versions.

Closes apache#1504
Signed-off-by: Marton Szasz <szaszm@apache.org>
Co-authored-by: Martin Zink <martinzink@apache.org>
lordgamez pushed a commit to lordgamez/nifi-minifi-cpp that referenced this pull request Mar 9, 2023
To support binaries targeting multiple libpython minor versions.

Closes apache#1504
Signed-off-by: Marton Szasz <szaszm@apache.org>
Co-authored-by: Martin Zink <martinzink@apache.org>
lordgamez pushed a commit to lordgamez/nifi-minifi-cpp that referenced this pull request Mar 9, 2023
To support binaries targeting multiple libpython minor versions.

Closes apache#1504
Signed-off-by: Marton Szasz <szaszm@apache.org>
Co-authored-by: Martin Zink <martinzink@apache.org>
lordgamez pushed a commit to lordgamez/nifi-minifi-cpp that referenced this pull request Mar 9, 2023
To support binaries targeting multiple libpython minor versions.

Closes apache#1504
Signed-off-by: Marton Szasz <szaszm@apache.org>
Co-authored-by: Martin Zink <martinzink@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants