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 import future annotations in venv jinja template #40208

Merged
merged 12 commits into from
Jun 14, 2024

Conversation

phi-friday
Copy link
Contributor

@phi-friday phi-friday commented Jun 13, 2024


The following Task, when rendered as a jinja template, will result in an error because Any is not imported

from __future__ import annotations
from typing import Any

@task.docker(image='some_image')
def some_task(value: dict[str, Any]) -> Any:
    ...

Changing it to a string like this will solve this problem, but it's a bit cumbersome.

from __future__ import annotations
from typing import Any

@task.docker(image='some_image')
def some_task(value: "dict[str, Any]") -> "Any":
    ...

It would be much easier if I could check if annotations is imported when rendering with jinja.

^ 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.

@phi-friday phi-friday force-pushed the fix-import-future-annotations branch 2 times, most recently from 3a4739b to 0a630a7 Compare June 13, 2024 01:37
@phi-friday phi-friday force-pushed the fix-import-future-annotations branch 4 times, most recently from f5372a0 to 60192ac Compare June 13, 2024 12:45
@potiuk
Copy link
Member

potiuk commented Jun 13, 2024

The test will have to be skipped on airflow pre 2.10 because it will not work there (See failing compatibility tests) and ways how to deal with the compatibility issues: https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#implementing-compatibility-for-provider-tests-for-older-airflow-versions

@phi-friday phi-friday force-pushed the fix-import-future-annotations branch from f1b5241 to b2edec0 Compare June 14, 2024 00:20
@phi-friday phi-friday force-pushed the fix-import-future-annotations branch from b2edec0 to da9afe2 Compare June 14, 2024 00:20
@phi-friday
Copy link
Contributor Author

First, I changed the test to only work with airflow>=2.10.
But looking at the output, I don't think it's a change to airflow[docker].

Should I change the test location and remove the airflow 2.10+ condition?

@phi-friday
Copy link
Contributor Author

Changed the location, as it affects venv operator.

@phi-friday phi-friday changed the title Fix import future annotations in docker decorator Fix import future annotations in venv jinja template Jun 14, 2024
@phi-friday phi-friday force-pushed the fix-import-future-annotations branch from 80afd4c to 32876b9 Compare June 14, 2024 00:53
@phi-friday
Copy link
Contributor Author

It passes all the tests, but maybe there's a problem with the wrong label.
@potiuk Could you change it and run action again?

@potiuk
Copy link
Member

potiuk commented Jun 14, 2024

It passes all the tests, but maybe there's a problem with the wrong label. @potiuk Could you change it and run action again?

Ah yeah - now it's not a "docker provider" test - it runs only in 'airflow core` and that's correct, because it affects all the decorators not only docker, and it should not be "provider" test as it was before.

@potiuk potiuk merged commit d5a7544 into apache:main Jun 14, 2024
51 checks passed
@eladkal eladkal added this to the Airflow 2.9.3 milestone Jun 14, 2024
@eladkal eladkal added type:bug-fix Changelog: Bug Fixes and removed area:core labels Jun 14, 2024
@phi-friday phi-friday deleted the fix-import-future-annotations branch June 24, 2024 01:07
utkarsharma2 pushed a commit that referenced this pull request Jul 2, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants