Skip to content

[airflow]: Import modules that has been moved to airflow providers (AIR303)#14764

Merged
MichaReiser merged 1 commit intoastral-sh:mainfrom
astronomer:AIR303
Dec 13, 2024
Merged

[airflow]: Import modules that has been moved to airflow providers (AIR303)#14764
MichaReiser merged 1 commit intoastral-sh:mainfrom
astronomer:AIR303

Conversation

@Lee-W
Copy link
Contributor

@Lee-W Lee-W commented Dec 4, 2024 •
edited
Loading

Summary

Many core Airflow features have been deprecated and moved to Airflow Providers since users might need to install an additional package (e.g., apache-airflow-provider-fab==1.0.0); a separate rule (AIR303) is created for this.

As some of the changes only relate to the module/package moved, instead of listing out all the functions, variables, and classes in a module or a package, it warns the user to import from the new path instead of the specific name.

The following is the ones that has been moved to apache-airflow-provider-fab==1.0.0

  • module moved
    • airflow.api.auth.backend.basic_auth → airflow.providers.fab.auth_manager.api.auth.backend.basic_auth
    • airflow.api.auth.backend.kerberos_auth → airflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth
    • airflow.auth.managers.fab.api.auth.backend.kerberos_auth → airflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth
    • airflow.auth.managers.fab.security_manager.override → airflow.providers.fab.auth_manager.security_manager.override
  • classes (e.g., functions, classes) moved
    • airflow.www.security.FabAirflowSecurityManagerOverride → airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride
    • airflow.auth.managers.fab.fab_auth_manager.FabAuthManager → airflow.providers.fab.auth_manager.security_manager.FabAuthManager

#14764

How to contribute to AIR303

Browse the airflow / ruff migration guideline

How to create a Ruff lint rule for Airflow 2-to-3 migrations

Setup local development environment

cargo install cargo-insta

uv tool install pre-commit
pre-commit install

cargo install cargo-nextest --locked

Contributing to Ruff

Check the existing rules from airflow issue

All the ruff rules are listed down in Airflow 2 to 3 auto migration rules - ruff #44556

Go to AIR303: moved to provider in the issue description and you'll find rules like

* `airflow.www.security.FabAirflowSecurityManagerOverride` → `airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride` (from https://github.com/apache/airflow/pull/41758)

It means airflow.www.security.FabAirflowSecurityManagerOverride has been moved to airflow provider and the import should be changed to airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride. This changed is made in apache/airflow#41758

Some of the rules might not be correct, so checking the pull request again is suggested.

Add a new rule

Go to crates/ruff_linter/src/rules/airflow/rules/moved_to_provider_in_3.rs

In function moved_to_provider, add code section like the following

                ["airflow", "www", "security", "FabAirflowSecurityManagerOverride"] => Some((
                    qualname.to_string(),
                    Replacement::ProviderName {
                        name: "airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride",
                        provider: "fab",
                        version: "1.0.0"
                    },
                )),
  • "airflow", "www", "security", "FabAirflowSecurityManagerOverride": the deprecated import path
  • name is the new name that should be used.
  • provider is which provider this name has been moved to.
  • version is the first version of the provider that has been moved to.

For rules like

* [ ] `airflow.api.auth.backend.basic_auth` → `airflow.providers.fab.auth_manager.api.auth.backend.basic_auth` (from https://github.com/apache/airflow/pull/41663)

The whole module/pacakge has been moved to the provider. Thus, we'll need to warn the users when something is imported from the old path instead.

                ["airflow", "api", "auth", "backend", "basic_auth", ..] => Some((
                    qualname.to_string(),
                    Replacement::ImportPathMoved{
                        original_path: "airflow.api.auth.backend.basic_auth",
                        new_path: "airflow.providers.fab.auth_manager.api.auth.backend.basic_auth",
                        provider:"fab",
                        version: "1.0.0"
                },
                )),
  • "airflow", "api", "auth", "backend", "basic_auth", ..: anything under this import path
  • original path
  • new path: new import path in the provider
  • provider is which provider this name has been moved to.
  • version is the first version of the provider that has been moved to.

Add test case

Add a case that violate this rule to crates/ruff_linter/resources/test/fixtures/airflow/AIR303.py

Test and generate snapshop to review

cargo insta test
cargo insta review

Now you're ready to create a pull request

Test Plan

A test fixture has been included for the rule.

@Lee-W Lee-W changed the title feat(AIR303): initial air303 [airflow]: Import modules that has been moved to airflow providers (AIR303) Dec 4, 2024
@Lee-W Lee-W mentioned this pull request Dec 4, 2024
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@Lee-W Lee-W force-pushed the AIR303 branch 2 times, most recently from 03288b2 to 46e0214 Compare December 10, 2024 11:04
@Lee-W Lee-W marked this pull request as ready for review December 10, 2024 11:05
@Lee-W Lee-W mentioned this pull request Dec 10, 2024
Closed
2 tasks
@Lee-W
Copy link
Contributor Author

Lee-W commented Dec 11, 2024

@MichaReiser would love to know whether the overall idea of this PR makes sense if you have some time. I plan on writing a guideline for folks from the airflow community to contribute to AIR303 more easily. 🙂

@dhruvmanila
Copy link
Member

dhruvmanila commented Dec 11, 2024

Is it correct that the entire module itself has been moved to being used as a provider without changing any of the public symbols available in that module? i.e., are the symbols that were available in the old module the same as that in the provider module? If so, we could consider the rule to work on the module instead of individual symbols from the module and suggest to use the provider instead. That way we'll only emit a single diagnostic for the entire module which will be the import statement.

@dhruvmanila dhruvmanila added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 11, 2024
@Lee-W
Copy link
Contributor Author

Lee-W commented Dec 11, 2024

Is it correct that the entire module itself has been moved to being used as a provider without changing any of the public symbols available in that module? i.e., are the symbols that were available in the old module the same as that in the provider module?

Yep, that's what I think for some of the modules, but I'll confirm it again.

If so, we could consider the rule to work on the module instead of individual symbols from the module and suggest to use the provider instead. That way we'll only emit a single diagnostic for the entire module which will be the import statement.

Yep, some of the modules can be done this way, but others might not, so that why I'd like to keep the individual symbols check as well

@dhruvmanila
Copy link
Member

dhruvmanila commented Dec 11, 2024

Yep, some of the modules can be done this way, but others might not, so that why I'd like to keep the individual symbols check as well

That's sounds reasonable.

MichaReiser
MichaReiser approved these changes Dec 11, 2024
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

writing a guideline for folks from the airflow community to contribute to AIR303 more easily.

This sounds great. I'd appreciate your with reviewing and coordinating the PRs as you have the most knowledge about what the rules should cover.

@Lee-W
Copy link
Contributor Author

Lee-W commented Dec 11, 2024

writing a guideline for folks from the airflow community to contribute to AIR303 more easily.

This sounds great. I'd appreciate your with reviewing and coordinating the PRs as you have the most knowledge about what the rules should cover.

Will do 👍

@Lee-W Lee-W force-pushed the AIR303 branch 4 times, most recently from f1399eb to 77e3c45 Compare December 13, 2024 08:53
@Lee-W
feat(AIR303): add rules for names moved to fab provider
4a6fec6 
* import path (module/package) moved
  * `airflow.api.auth.backend.basic_auth` → `airflow.providers.fab.auth_manager.api.auth.backend.basic_auth`
  * `airflow.api.auth.backend.kerberos_auth` → `airflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth`
  * `airflow.auth.managers.fab.api.auth.backend.kerberos_auth` → `airflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth`
  * `airflow.auth.managers.fab.security_manager.override` → `airflow.providers.fab.auth_manager.security_manager.override`
* other names (e.g., functions, classes) moved
  * `airflow.www.security.FabAirflowSecurityManagerOverride` → `airflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride`
  * `airflow.auth.managers.fab.fab_auth_manager.FabAuthManager` → `airflow.providers.fab.auth_manager.security_manager.FabAuthManager`
@MichaReiser MichaReiser merged commit dfd7f38 into astral-sh:main Dec 13, 2024
dcreager added a commit that referenced this pull request Dec 13, 2024
@dcreager
Merge branch 'main' into dcreager/module-resolution
80f65b7 
* main:
  [red-knot] Display definition range in trace logs (#14955)
  [red-knot] Move the `ClassBase` enum to its own submodule (#14957)
  [red-knot] mdtest: python version requirements (#14954)
  [airflow]: Import modules that has been moved to airflow providers (AIR303) (#14764)
  [red-knot] Support `typing.TYPE_CHECKING` (#14952)
  Add tracing support to mdtest (#14935)
  Re-enable the fuzzer job on PRs (#14953)
  [red-knot] Improve `match` mdtests (#14951)
  Rename `custom-typeshed-dir`, `target-version` and `current-directory` CLI options (#14930)
  [red-knot] Add narrowing for 'while' loops (#14947)
  [`ruff`]  Skip SQLModel base classes for `mutable-class-default` (`RUF012`) (#14949)
  [red-knot] Tests for 'while' loop boundness (#14944)
@Lee-W Lee-W deleted the AIR303 branch December 18, 2024 00:07
@BrewTestBot BrewTestBot mentioned this pull request Dec 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhruvmanila dhruvmanila dhruvmanila left review comments

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Lee-W @dhruvmanila @MichaReiser