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

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

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

Lee-W
Copy link
Contributor

@Lee-W Lee-W commented Dec 4, 2024

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_authairflow.providers.fab.auth_manager.api.auth.backend.basic_auth
    • airflow.api.auth.backend.kerberos_authairflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth
    • airflow.auth.managers.fab.api.auth.backend.kerberos_authairflow.providers.fab.auth_manager.api.auth.backend.kerberos_auth
    • airflow.auth.managers.fab.security_manager.overrideairflow.providers.fab.auth_manager.security_manager.override
  • classes (e.g., functions, classes) moved
    • airflow.www.security.FabAirflowSecurityManagerOverrideairflow.providers.fab.auth_manager.security_manager.override.FabAirflowSecurityManagerOverride
    • airflow.auth.managers.fab.fab_auth_manager.FabAuthManagerairflow.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
Copy link
Contributor

github-actions bot commented Dec 9, 2024

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

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

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.

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
* 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
21 checks passed
dcreager added a commit that referenced this pull request Dec 13, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants