Enhance Python SDK: Implement Robust Error Handling for add_output Function #687
Conversation
sp6370
requested review from
saqadri,
rholinshead,
suyoglastmileai,
Ankush-lastmile,
jonathanlastmileai and
rossdanlm
as code owners
sp6370
changed the title
add_ouputadd_ouput
sp6370
changed the title
add_ouputadd_ouput in Python SDK
sp6370
changed the title
add_ouput in Python SDKadd_ouput in Python SDK
| # Case 1: No output, no overwrite | ||
| with pytest.raises( | ||
| Exception, | ||
| match=r"Cannot add output to prompt 'GreetingPrompt'. Output is empty.", |
There was a problem hiding this comment.
nit: to make things easier, would make "GreetingPrompt" it's own var like prompt_name and just re-use it here. Not a big deal though
There was a problem hiding this comment.
I would stick to keeping it as it is now. Since isn't being used anymore in the test.
|
Approaching this with curiosity. What's the best approach to managing PR comments? Should the author address the comment and mark it as resolved, or is it the responsibility of the reviewer to close out the comments after an update or commit? |
Personally I would say the PR author addresses comments and resolves as they're fixed. Some people prefer that reviewer be more strict and review everything, but I feel that reviewer should just give another final look to confirm everything is cool anyways |
|
This looks great! Thanks for being so diligent with testing and fixes from comments! Looking forward to continue working with you :) |
sp6370
changed the title
add_ouput in Python SDK
Summary:
This pull request addresses a critical flaw in the add_output method within the Python SDK. Currently, the method incorrectly adds None to the output list when the Output parameter is None, which contradicts the intended behavior. The expected functionality is for the method to reject None as an invalid output or, alternatively, raise an exception.
Changes Made:
Context:
This change ensures that the add_output method behaves as expected, providing more robust and predictable behavior when handling output parameters.
Related Issues:
Closes #529
Test Plan:
New test similar to
test_add_output_existing_prompt_no_overwritebut with overwrite needs to be added.Things left to complete:
add_outputwith overwrite enabled