Skip to content

[airflow] Apply auto fixes to cases where the names have changed in Airflow 3 (AIR301) - #17355

Merged
ntBre merged 2 commits into
astral-sh:mainfrom
astronomer:apply-auto-import
Apr 23, 2025
Merged

ntBre merged 2 commits into
astral-sh:mainfrom
astronomer:apply-auto-import

Conversation

@Lee-W

@Lee-W Lee-W commented Apr 11, 2025 •

Copy link
Copy Markdown
Contributor

Summary

Apply auto fixes to cases where the names have changed in Airflow 3

Test Plan

Add AIR301_names_fix.py and AIR301_provider_names_fix.py test fixtures

@Lee-W

Lee-W commented Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Hey @ntBre , I found out a Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics: error during testing. but the traceback does not provide much information. Do you know what the cause might be? thanks!

@ntBre

ntBre commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

Hey @ntBre , I found out a Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics: error during testing. but the traceback does not provide much information. Do you know what the cause might be? thanks!

That usually points to a loop between two rules, for example one rule might add an import and the other deletes it. Based on the test that's failing, it probably only has one rule (AIR301) enabled, so it would probably have to be a loop within that rule. Do any of the replacements overlap? Something like

import a  # error, import b instead

Fixed to

import b  # error, import a instead

Fixed to

import a

and so on. There could even be more parts in the chain like a -> b -> c -> a, which could make it harder to identify, but something like this should be the cause.

@Lee-W

Lee-W commented Apr 12, 2025

Copy link
Copy Markdown
Contributor Author

Hey @ntBre , I found out a Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics: error during testing. but the traceback does not provide much information. Do you know what the cause might be? thanks!

That usually points to a loop between two rules, for example one rule might add an import and the other deletes it. Based on the test that's failing, it probably only has one rule (AIR301) enabled, so it would probably have to be a loop within that rule. Do any of the replacements overlap? Something like

import a  # error, import b instead

Fixed to

import b  # error, import a instead

Fixed to

import a

and so on. There could even be more parts in the chain like a -> b -> c -> a, which could make it harder to identify, but something like this should be the cause.

Hmm... don't think we have such case. but will take a deeper look.

@dhruvmanila

Copy link
Copy Markdown
Member

I'm not sure how the fix has been implemented but a potential reason this might be happening is there are multiple autofixes (more than the iteration limit) which is trying to edit the same line of import. This could happen when there are multiple references of the same deprecated symbol and each reference has generated an edit to update the import. Now, that edit belongs to the same line so Ruff will only apply a single edit and avoid fixing the rest. Those will then be carried over to the next iteration. I guess this is why the autoimporter logic created an error. But, I'll need to look closely as to why this is happening.

It might be worth printing out some of the variables in the fix loop to see why the linter is not able to apply the fixes in a single loop.

(Posting from phone)

@Lee-W

Lee-W commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

I'm not sure how the fix has been implemented but a potential reason this might be happening is there are multiple autofixes (more than the iteration limit) which is trying to edit the same line of import. This could happen when there are multiple references of the same deprecated symbol and each reference has generated an edit to update the import. Now, that edit belongs to the same line so Ruff will only apply a single edit and avoid fixing the rest. Those will then be carried over to the next iteration. I guess this is why the autoimporter logic created an error. But, I'll need to look closely as to why this is happening.

It might be worth printing out some of the variables in the fix loop to see why the linter is not able to apply the fixes in a single loop.

(Posting from phone)

I've not yet had the bandwidth to come back to this one. 😞 But multiple fix on the same line sounds like a highly likely cause 🤔

@Lee-W
Lee-W force-pushed the apply-auto-import branch 2 times, most recently from 2f3baf1 to 6d6e414 Compare April 21, 2025 14:50
@Lee-W Lee-W changed the title Apply auto import [airflow] Apply fixes to cases that names that have been changed in Airflow 3 (AIR301) Apr 21, 2025
@Lee-W
Lee-W force-pushed the apply-auto-import branch from 6d6e414 to 51b81b3 Compare April 21, 2025 14:55
@Lee-W Lee-W changed the title [airflow] Apply fixes to cases that names that have been changed in Airflow 3 (AIR301) [airflow] Apply fixes to cases that names changed in Airflow 3 (AIR301) Apr 21, 2025
@Lee-W Lee-W changed the title [airflow] Apply fixes to cases that names changed in Airflow 3 (AIR301) [airflow] Apply auto fixes to cases where the names have changed in Airflow 3 (AIR301) Apr 21, 2025
@Lee-W
Lee-W marked this pull request as ready for review April 21, 2025 14:57
@Lee-W

Lee-W commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

Hey, @ntBre I just wrap up this one. Would be nice if you could take a look when you have time. Thanks 🙂

@github-actions

github-actions Bot commented Apr 21, 2025 •

Copy link
Copy Markdown
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -1 violations, +10 -0 fixes in 1 projects; 54 projects unchanged)

apache/airflow (+0 -1 violations, +10 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:32: AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:32: AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:5: AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:5: AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:63:17: AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:63:17: AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:67:17: AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:67:17: AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- providers/common/compat/src/airflow/providers/common/compat/openlineage/utils/utils.py:40:59: AIR301 `airflow.providers.openlineage.utils.utils.translate_airflow_dataset` is removed in Airflow 3.0
+ providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:580:21: AIR301 [*] `airflow.timetables.simple.DatasetTriggeredTimetable` is removed in Airflow 3.0
- providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:580:21: AIR301 `airflow.timetables.simple.DatasetTriggeredTimetable` is removed in Airflow 3.0

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
AIR301 11 0 1 10 0

@ntBre ntBre self-assigned this Apr 21, 2025

@ntBre ntBre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me!

@ntBre ntBre added fixes Related to suggested fixes for violations preview Related to preview-mode features labels Apr 21, 2025
@ntBre

ntBre commented Apr 21, 2025

Copy link
Copy Markdown
Contributor

I guess one quick question about the ecosystem check: airflow.providers.openlineage.utils.utils.translate_airflow_dataset is no longer being detected/fixed. Is that expected?

Otherwise the ecosystem check looks good, it shows new fixes for the other matches.

@Lee-W

Lee-W commented Apr 22, 2025

Copy link
Copy Markdown
Contributor Author

I guess one quick question about the ecosystem check: airflow.providers.openlineage.utils.utils.translate_airflow_dataset is no longer being detected/fixed. Is that expected?

Otherwise the ecosystem check looks good, it shows new fixes for the other matches.

I think it's fixed here? https://github.com/astral-sh/ruff/pull/17355/files#diff-493d19bbac615454a597e22069204d09be7c4e125ce6494d9cd85289278b141fR231

or are you referring to something else

@ntBre

ntBre commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

I'm talking about this entry from the ecosystem check: https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/openlineage/utils/utils.py#L40

All of the other entries have one new diagnostic (+) and one removed diagnostic (-), but this one shows only a removed diagnostic:

+ [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:32:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L39) AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:32:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L39) AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:5:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L39) AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:39:5:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L39) AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:63:17:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L63) AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:63:17:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L63) AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
+ [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:67:17:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L67) AIR301 [*] `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
- [providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py:67:17:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/lineage/hook.py#L67) AIR301 `airflow.lineage.hook.DatasetLineageInfo` is removed in Airflow 3.0
this one -> - [providers/common/compat/src/airflow/providers/common/compat/openlineage/utils/utils.py:40:59:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/common/compat/src/airflow/providers/common/compat/openlineage/utils/utils.py#L40) AIR301 `airflow.providers.openlineage.utils.utils.translate_airflow_dataset` is removed in Airflow 3.0
+ [providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:580:21:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/openlineage/tests/unit/openlineage/plugins/test_utils.py#L580) AIR301 [*] `airflow.timetables.simple.DatasetTriggeredTimetable` is removed in Airflow 3.0
- [providers/openlineage/tests/unit/openlineage/plugins/test_utils.py:580:21:](https://github.com/apache/airflow/blob/689608d7423cebf7c72d0e0c0081eceeeac51b3a/providers/openlineage/tests/unit/openlineage/plugins/test_utils.py#L580) AIR301 `airflow.timetables.simple.DatasetTriggeredTimetable` is removed in Airflow 3.0

I just wanted to make sure that was an expected result before merging.

@Lee-W
Lee-W force-pushed the apply-auto-import branch from 51b81b3 to 1bb93e7 Compare April 23, 2025 00:25
@Lee-W

Lee-W commented Apr 23, 2025 •

Copy link
Copy Markdown
Contributor Author

If I'm not mistaken, it seems we'll be running this branch against the Airflow codebase.I think the behavior is correct in this version as the code is guarded by try-catch. I'm not sure why it was there though 🤔

@ntBre

ntBre commented Apr 23, 2025

Copy link
Copy Markdown
Contributor

Sounds good, I'll go ahead and merge. Just wanted to make sure that was expected 🙂

@ntBre
ntBre merged commit b537552 into astral-sh:main Apr 23, 2025
dcreager added a commit that referenced this pull request Apr 23, 2025
…tructor

* origin/main:
  [red-knot] Trust module-level undeclared symbols in stubs (#17577)
  [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355)
  [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090)
  [red-knot] Detect semantic syntax errors (#17463)
  Fix stale diagnostics in Ruff playground (#17583)
  [red-knot] Early return from `project.is_file_open` for vendored files (#17580)
dcreager added a commit that referenced this pull request Apr 23, 2025
…var-instance

* dcreager/generic-constructor:
  Revert FunctionLiteral type
  [red-knot] Trust module-level undeclared symbols in stubs (#17577)
  [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR301`) (#17355)
  [`pycodestyle`] Auto-fix redundant boolean comparison (`E712`) (#17090)
  Clean this up a bit
  clippy
  [red-knot] Detect semantic syntax errors (#17463)
  Fix stale diagnostics in Ruff playground (#17583)
  [red-knot] Early return from `project.is_file_open` for vendored files (#17580)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixes Related to suggested fixes for violations preview Related to preview-mode features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants