Skip to content
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

Ensure deps is set, convert BaseSensorOperator to classvar #21815

Merged
merged 3 commits into from
Mar 2, 2022

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Feb 25, 2022

Fix #21807?

This is the easiest way to get mapped operator's deps to work without jumping through too many hoops. This does introduce some potential backward incompatibility for peopel relying on the (internal?) dep class mechanism, but I think it is acceptable.

@uranusjr uranusjr requested a review from ashb February 25, 2022 05:48
@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Feb 25, 2022
@uranusjr
Copy link
Member Author

This gets me pass the error, but hits another one:

Traceback (most recent call last):
  File "dags/t.py", line 23, in <module>
    target_time=templates_xcomarg
  File "/opt/airflow/airflow/models/mappedoperator.py", line 215, in apply
    end_date=end_date,
  File "<attrs generated init airflow.models.mappedoperator.MappedOperator>", line 24, in __init__
  File "/opt/airflow/airflow/models/mappedoperator.py", line 259, in __attrs_post_init__
    self._validate_argument_count()
  File "/opt/airflow/airflow/models/mappedoperator.py", line 300, in _validate_argument_count
    op = self._create_unmapped_operator(mapped_kwargs=mocked_mapped_kwargs, real=False)
  File "/opt/airflow/airflow/models/mappedoperator.py", line 453, in _create_unmapped_operator
    **mapped_kwargs,
  File "/opt/airflow/airflow/models/baseoperator.py", line 374, in apply_defaults
    result = func(self, **kwargs, default_args=default_args)
  File "/opt/airflow/airflow/sensors/date_time.py", line 69, in __init__
    f"Expected str or datetime.datetime type for target_time. Got {type(target_time)}"
TypeError: Expected str or datetime.datetime type for target_time. Got <class 'unittest.mock.MagicMock'>

I guess we do need a class-level validate hook on operators.

@uranusjr uranusjr changed the title Male BaseSensorOperator.deps a class variable Make BaseSensorOperator.deps a class variable Feb 25, 2022
@uranusjr uranusjr force-pushed the reschedule-dep-checks-attr branch from 1c5905f to b01eb5b Compare February 25, 2022 05:54
@ashb
Copy link
Member

ashb commented Feb 25, 2022

This fixes the current issue, but may break/not work with customer operators if someone sets deps on an instance level :(

@ashb
Copy link
Member

ashb commented Feb 25, 2022

TypeError: Expected str or datetime.datetime type for target_time. Got <class 'unittest.mock.MagicMock'>


I guess we do need a class-level `validate` hook on operators.

We're going to hit this sort of error more and more with other operators. We might need to rethink how we validate the args

@uranusjr
Copy link
Member Author

may break/not work with customer operators if someone sets deps on an instance level :(

Maybe we can detect that (easy enough, simply check the attribute is actually a set/collection on the class level and not a descriptor), and raise an error saying the operator needs to be fixed? This can likely be done on DAG-parsing time.

@ashb
Copy link
Member

ashb commented Feb 28, 2022

may break/not work with customer operators if someone sets deps on an instance level :(

Maybe we can detect that (easy enough, simply check the attribute is actually a set/collection on the class level and not a descriptor), and raise an error saying the operator needs to be fixed? This can likely be done on DAG-parsing time.

👍 I think it's unlikely enough to affect many people, so if we put a note in updating about this it should be good enough

@evgenyshulman I remember on the multi-tenancy call that you do this. Would this cause problems for you?

@uranusjr uranusjr force-pushed the reschedule-dep-checks-attr branch from b01eb5b to 9d00d5f Compare February 28, 2022 12:32
@uranusjr
Copy link
Member Author

Check added.

@uranusjr uranusjr changed the title Make BaseSensorOperator.deps a class variable Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable Feb 28, 2022
@uranusjr uranusjr force-pushed the reschedule-dep-checks-attr branch from f47251d to 8e61c57 Compare February 28, 2022 13:04
uranusjr added 3 commits March 1, 2022 21:23
This is the easiest way to get mapped operator's deps to work without
jumping through too many hoops. This does introduce some potential
backward incompatibility for peopel relying on the (internal?) dep class
mechanism, but I _think_ it is acceptable.
@uranusjr uranusjr force-pushed the reschedule-dep-checks-attr branch from 8e61c57 to 9132d7a Compare March 2, 2022 10:45
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 2, 2022
@github-actions
Copy link

github-actions bot commented Mar 2, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr uranusjr changed the title Ensure a mappable operator's deps is a set, and convert BaseSensorOperator.deps to a class variable Ensure deps is set, convert BaseSensorOperator to classvar Mar 2, 2022
@uranusjr uranusjr merged commit 8b276c6 into apache:main Mar 2, 2022
@uranusjr uranusjr deleted the reschedule-dep-checks-attr branch March 2, 2022 14:19
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:dynamic-task-mapping AIP-42 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamically mapped sensors throw TypeError at DAG parse time
4 participants