Skip to content

Restore argument-provision checks to __init__ in operators fixed by the burn-down #70503

Description

@shahar1

Background

validate-operators-init flags every read of a template-field parameter in __init__. A companion PR narrows that: a field is None / field is not None check reads whether the argument was passed, not its value, and belongs in the constructor. (With render_template_as_native_obj=True a provided field renders to None, so the same check in execute() reports a supplied argument as missing; and raising at construction surfaces a static authoring mistake as a Dag import error rather than per task instance.)

Replaying both checker versions over every exemption entry removed so far (31 PRs, 48 entries) shows 8 entries across 5 PRs where the old rule forced a change that was not needed. Those PRs are merged and green — nothing is broken — but the files now carry comments and regression tests asserting the placement the docs discourage, and contributors working #70296 learn the rule by reading them.

This issue tracks putting them back. Each is independent; separate PRs are fine.

Blocked on #70505. Until it merges, validate-operators-init still flags every read of a template-field parameter in __init__, so a restored check fails the hook — a revert landed earlier would have to re-add the exemption entry it just removed.

Revert

Split, don't revert

  • providers/common/ai/.../operators/document_loader.py (Validate template fields after rendering in common.ai operators #70338) — the source_path/source_bytes exclusivity is provision and returns to __init__. The third check, source_bytes is not None and file_type is None, backs the downstream assert self.file_type is not None; file_type is a template field, so that assumption only holds after rendering and it stays in execute(). Update the comment to say which is which — this is the canonical example of the boundary.

Rewrite, don't revert

The replay above only finds reverts that are free — entries whose findings drop to zero with no edit. Provision checks written as truthiness (if a and b: raise) score the same under both checkers, so they never showed up in it, but they are the same kind of check and moving them to execute() cost more: on rendered values a mutual-exclusion check silently accepts the conflict when one side renders empty. Restoring these means rewriting and to is not None and, which is strictly stricter — it also rejects field="" alongside a second argument, and both were supplied.

Leave alone (listed so nobody reverts them by pattern-match)

Required-argument checks written as truthiness stay in execute() too. if not field: raise there still raises — it is later feedback, never a silent accept — and rewriting it to if field is None: raise in __init__ would loosen it, letting field="" through. Splitting each into a provision check plus a value check is more churn than the placement is worth:

Not in scope

The other 40 exemption entries removed so far are genuine value reads (ruleset.strip(), isinstance(mongo_query, list), source not in SUPPORTED_SOURCES, ...). They stay flagged under the narrowed rule and must not be reverted.

Verifying a revert

Run the hook on the file after the change — all classes above are clean under the narrowed rule, so no exemption entries need re-adding:

uv run --script scripts/ci/prek/validate_operators_init.py <path>

related: #70296


Drafted-by: Claude Code (Opus 5) (no human review before posting)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:providerskind:metaHigh-level information important to the community

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions