AP-25704: add inactive output support for Python nodes#92
AP-25704: add inactive output support for Python nodes#92
Conversation
AtR1an
requested review from
Copilot and
knime-ghub-bot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR introduces a first-class “inactive output” marker for Python-based KNIME nodes and wires it through the Python↔Java port conversion layer so nodes can explicitly mark specific output slots as inactive.
Changes:
- Add
knext.InactivePortsentinel and document its use inconfigure()/execute(). - Map the sentinel to/from KNIME’s
InactiveBranchPortObject/InactiveBranchPortObjectSpecin both the Python backend launcher and Java port type registry. - Add Python and Java tests covering spec/object conversion and proxy behavior while preserving output arity.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
org.knime.python3.nodes/src/main/python/knime/extension/nodes.py |
Adds InactivePort sentinel + API doc updates for returning inactive outputs. |
org.knime.python3.nodes/src/main/python/knime/extension/__init__.py |
Re-exports InactivePort on the public knime.extension surface. |
org.knime.python3.nodes/src/main/python/_node_backend_launcher.py |
Converts inactive specs/objects between Python marker and KNIME inactive port types (by class name). |
org.knime.python3.nodes/src/main/java/org/knime/python3/nodes/ports/PythonPortTypeRegistry.java |
Adds Java-side conversion shortcuts for inactive port object/spec. |
org.knime.python3.nodes/src/main/java/org/knime/python3/nodes/ports/PythonPortObjects.java |
Introduces Java-side Python wrapper singletons for inactive port object/spec. |
org.knime.python3.nodes.tests/src/test/python/unittest/test_knime_node_backend.py |
Adds unit tests for inactive conversions and proxy output arity. |
org.knime.python3.nodes.tests/src/test/java/org/knime/python3/nodes/ports/InactivePortConversionTest.java |
Adds Java unit tests for inactive conversions. |
| Either a single spec, or a tuple or list of specs. The number of specs | ||
| must match the number of defined output ports, and they must be returned in this order. | ||
| Alternatively, instead of a spec, a knext.Column can be returned (if the spec shall | ||
| only consist of one column). | ||
| only consist of one column). Return `knext.InactivePort` for an output slot to | ||
| mark that port inactive. |
There was a problem hiding this comment.
The docstring only describes returning knext.InactivePort for outputs, but the backend also converts inactive incoming port specs/objects to knext.InactivePort (so *inputs in configure() / execute() can be this marker when an upstream branch is inactive). Consider documenting this input behavior as well, so node authors know they may need to handle/forward inactive inputs.
There was a problem hiding this comment.
Interesting remark. Are nodes executed or configured at all if they have inactive input ports? If not, then no need to explain that
There was a problem hiding this comment.
Only nodes with a NodeModel implementing InactiveBranchConsumer get configured/executed when the branch is inactive (e.g. Case End). The NodeModel of Python-based nodes does not implement that interface, so this can currently not happen. Question is if we should then also remove the Java -> Python direction since it can't currently happen?
AtR1an
force-pushed
the
enh/AP-25704-inactive-ports
branch
from
cdb68f6 to
488e201
Compare
|
chaubold
left a comment
chaubold
left a comment
There was a problem hiding this comment.
If I'm not mistaken then we can achieve the same thing on the Java side without additional if blocks in all the conversion methods, by registering a built-in converter, right?
| } | ||
|
|
||
| if (spec instanceof InactiveBranchPortObjectSpec) { | ||
| return PythonInactivePortObjectSpec.INSTANCE; |
There was a problem hiding this comment.
why do we add an extra check here instead of registering a converter for the inactive branches and put that into m_builtinPortObjectSpecConverterMap?
| var instance = InstanceHolder.INSTANCE; | ||
| String specClassName = pythonSpec.getJavaClassName(); | ||
| if (InactiveBranchPortObjectSpec.class.getName().equals(specClassName)) { | ||
| return InactiveBranchPortObjectSpec.INSTANCE; |
There was a problem hiding this comment.
same here, it could be covered by the code below if there was an entry in m_builtinPortObjectSpecConverterMap?
| } | ||
|
|
||
| if (portObject instanceof InactiveBranchPortObject) { | ||
| return PythonInactivePortObject.INSTANCE; |
There was a problem hiding this comment.
this could also work via the m_builtinPortObjectConverterMap, right?
No description provided.