kaxil commented on code in PR #70628:
URL: https://github.com/apache/airflow/pull/70628#discussion_r3668141100
##########
providers/common/ai/src/airflow/providers/common/ai/operators/document_loader.py:
##########
@@ -148,11 +152,10 @@ def __init__(
self.json_text_field = json_text_field
def execute(self, context: Context) -> list[dict[str, Any]]:
- # source_path/file_type are template fields; validate after rendering,
not in __init__.
- if self.source_path is not None and self.source_bytes is not None:
- raise ValueError("Provide exactly one of 'source_path' or
'source_bytes', not both.")
- if self.source_path is None and self.source_bytes is None:
- raise ValueError("Provide exactly one of 'source_path' or
'source_bytes'.")
+ # 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__.
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').")
Review 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.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/document_loader.py:
##########
@@ -148,11 +152,10 @@ def __init__(
self.json_text_field = json_text_field
def execute(self, context: Context) -> list[dict[str, Any]]:
- # source_path/file_type are template fields; validate after rendering,
not in __init__.
- if self.source_path is not None and self.source_bytes is not None:
- raise ValueError("Provide exactly one of 'source_path' or
'source_bytes', not both.")
- if self.source_path is None and self.source_bytes is None:
- raise ValueError("Provide exactly one of 'source_path' or
'source_bytes'.")
+ # 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__.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
