Restore argument-provision checks to __init__ in common.ai operators - #70628
Restore argument-provision checks to __init__ in common.ai operators#70628baha-bouali wants to merge 7 commits into
Conversation
| # source_path/source_bytes provision is checked in __init__ (that's just "was an | ||
| # argument passed"). file_type is different: it backs the assert below, and since | ||
| # file_type is itself a template field, whether it was *actually* supplied is only | ||
| # knowable after rendering -- so this one check has to stay here, not in __init__. |
There was a problem hiding this comment.
The rationale here is inverted -- it repeats the claim #70505 refuted. Provision is not knowable after rendering: with render_template_as_native_obj=True a supplied field renders to None, which is precisely why the rule was narrowed to allow is None back in __init__.
What actually keeps this one in execute() is that it isn't a provision check at all, it's a value check: _parse_bytes needs a real extension string, so it has to see the rendered value. The PR body words it correctly ("checking it in __init__ would validate the unrendered template string instead") -- worth pulling that wording into the comment, since #70503 makes this file a reference others will copy.
| # file_type is itself a template field, whether it was *actually* supplied is only | ||
| # knowable after rendering -- so this one check has to stay here, not in __init__. | ||
| if self.source_bytes is not None and self.file_type is None: | ||
| raise ValueError("'file_type' is required when using 'source_bytes' (e.g. '.pdf').") |
There was a problem hiding this comment.
file_type keeps a rendered-value guard here, but source_path no longer has one, and the assert self.source_path is not None below was leaning on the check that just moved to __init__.
A supplied source_path that renders to None (the native-obj case #70505 is built around) now falls into the else branch, and _resolve_files(None) raises TypeError: argument of type 'NoneType' is not iterable instead of the previous clear ValueError. Worth keeping a self.source_path is None guard here next to the file_type one.
There was a problem hiding this comment.
I think it is fixed now. I reworded the comment and added back the source_path guard, thanks for catching this! Let me know if something is still wrong
Clarify comments regarding file_type and source_path checks in execute method.
| if self.source_bytes is None and self.source_path is None: | ||
| raise ValueError("Provide exactly one of 'source_path' or 'source_bytes'.") |
There was a problem hiding this comment.
Could you please add a unit test for this case?
There was a problem hiding this comment.
Done! I added the unit test right in test_document_loader.py.
3 participants
Restores two
__init__argument-provision checks that moved toexecute()under theold
validate-operators-initrule, now that #70505 narrowed the rule to allowfield is None/field is not Noneprovision checks back in__init__.AgentOperator: straight revert of #70338'smessage_history/enable_hitl_reviewcheck back to
__init__, withtest_agent.py::test_message_history_with_hitl_review_raisesreverted to assert at construction time (and its unneeded
skipifremoved).DocumentLoaderOperator: partial revert.source_path/source_bytesexclusivity ispure provision and moves back to
__init__. Thefile_typecheck stays inexecute(),since it backs an
asserton the rendered value offile_type(itself a templatefield). So, checking it in
__init__would validate the unrendered template string instead.Tests split to match: the two exclusivity tests revert to constructor-time, the two
file_typetests stay at.execute().Testing
uv run --script scripts/ci/prek/validate_operators_init.pyon both files -> clean,no exemption entries needed under the narrowed rule.
pytest providers/common/ai/tests/unit/common/ai/operators/test_agent.py providers/common/ai/tests/unit/common/ai/operators/test_document_loader.py-> passing.related: #70503
Was generative AI tooling used to co-author this PR?