-
Notifications
You must be signed in to change notification settings - Fork 18
[APO-2731] Filter undefined values from subworkflow inputs #3586
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| assert initiated_events[1].body.workflow_definition.is_dynamic is True # Inline workflow | ||
|
|
||
|
|
||
| def test_inline_subworkflow_node__undefined_input_filtered(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI remove this test and associated implementation, we want to focus on the deployment variants
| deployment = "example_subworkflow_deployment" | ||
| subworkflow_inputs = { | ||
| "required_input": "hello", | ||
| "optional_input": undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI remove this input from being defined
| ] | ||
| yield from expected_events | ||
|
|
||
| vellum_client.execute_workflow_stream.side_effect = generate_subworkflow_events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevinAI we want to mock the pull api so we can test the resolved flow and not the execute_workflow_stream
When optional subworkflow inputs have dependencies that resolve to undefined, the workflow should execute seamlessly using default values instead of raising INVALID_INPUTS errors. This change filters out undefined values in both SubworkflowDeploymentNode and InlineSubworkflowNode input compilation methods. Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
…and update test Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
f5dd0a4 to
90070a1
Compare
|
DevinAI can you run the test you added here against the main branch and show me what happens when you do? |
|
I ran the test against main and it passes: This means the test currently only proves "resolved workflow path works when the optional input is omitted from the dict" - which already works on main. It does not exercise the To actually demonstrate the bug fix, the test would need to create a situation where
|
|
DevinAI I would like you to revert the changes to |
|
I checked the PR diff and commit history - there are no changes to
I'll now iterate on the test to create one that fails on main. I'll set up a scenario where |
Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
|
I've updated the test to properly demonstrate the bug fix. The new test
On main: The test fails because On this branch: The test passes because This test exercises the actual bug fix in |
Fixes an issue where subworkflow deployments with optional inputs whose dependencies resolve to
undefinedwere raisingINVALID_INPUTSerrors instead of executing seamlessly using default values. The fix filters outundefinedvalues (in addition toNone) in the input compilation methods forSubworkflowDeploymentNode.Review & Testing Checklist for Human
test_compile_subworkflow_inputs__undefined_input_filteredfails on main (produces 2 inputs with undefined serialized as empty string) and passes on this branch (produces 1 input with undefined filtered out)Test Plan
pytest src/vellum/workflows/nodes/displayable/subworkflow_deployment_node/tests/test_node.py::test_compile_subworkflow_inputs__undefined_input_filtered -xvson this branch - should passNotes