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

Fix region argument in MappedOperator based on AwsBaseOperator / AwsBaseSensor #38178

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

Taragolis
Copy link
Contributor

Originally reported in Slack

Hi everyone! :nyanparrot:
I just upgraded the airflow packages versions:

apache-airflow==2.8.3
apache-airflow-providers-amazon==8.19.0

And now I have a broken DAG:

Broken DAG: [/opt/airflow/dags/distributor_catalogue.py]
Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.11/site-packages/airflow/models/baseoperator.py", line 265, in partial
    validate_mapping_kwargs(operator_class, "partial", kwargs)
  File "/home/airflow/.local/lib/python3.11/site-packages/airflow/models/mappedoperator.py", line 117, in validate_mapping_kwargs
    raise TypeError(f"{op.__name__}.{func}() got {error}")
TypeError: EcsRunTaskOperator.partial() got an unexpected keyword argument 'region'

The use of region is this one:

EcsRunTaskOperator.partial(
    task_id="task_id",
    aws_conn_id="aws_manual_test",
    cluster="tf_ecs_airflow",
    task_definition="tf_ecs_airflow",
    launch_type="FARGATE",
    region="eu-west-3",
    network_configuration={
        "awsvpcConfiguration": {
            "securityGroups": [Variable.get("ecs_securitygroup")],
            "subnets": [Variable.get("ecs_subnet1"), Variable.get("ecs_subnet2"), Variable.get("ecs_subnet3")],
            "assignPublicIp": "ENABLED",
        },
    },
    trigger_rule="all_done",
    waiter_delay=60,
    waiter_max_attempts=3000,
).expand(overrides=generate_params_list)

Does someone know why it is broken?
Thanks a lot 🥹 (edited)


This happen because partial check signature, and if argument not exists in any MRO references it will failed.
We grab region directly from kwargs and remove it, for avoid provide it into the BaseOperator.

Fix pretty simple, add region into AwsBaseOperator / AwsBaseSensor signatures, it should fix all operators which migrated to this helper operators and use deprecated region attribute over Dynamic Task Mapping


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Mar 15, 2024
@Taragolis Taragolis closed this Mar 15, 2024
@Taragolis Taragolis reopened this Mar 15, 2024
@Taragolis
Copy link
Contributor Author

Yeah... full test works fine. Except mypy, but it not related to this PR

@Taragolis
Copy link
Contributor Author

Taragolis commented Mar 15, 2024

Fingers crossed, seems like this test should work (without side effects)

@Taragolis Taragolis changed the title Fix region argument in partial for bases on AwsBaseOperator / AwsBaseSensor Fix region argument in MappedOperator based on AwsBaseOperator / AwsBaseSensor Mar 16, 2024
@Taragolis Taragolis merged commit 6029c71 into apache:main Mar 18, 2024
89 checks passed
@Taragolis Taragolis deleted the aws-base-on-partial branch March 18, 2024 17:53
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
…/ `AwsBaseSensor` (apache#38178)

* Fix `region` argument in partial for bases on `AwsBaseOperator` / `AwsBaseSensor`

* Add EcsRunTaskOperator.partial test

* Revert "Add EcsRunTaskOperator.partial test"

This reverts commit 32d0964.

* Second attempt with patial EcsRunTaskOperator tests

* Add missing session in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants