potiuk commented on code in PR #53627: URL: https://github.com/apache/airflow/pull/53627#discussion_r2225690054
########## pyproject.toml: ########## @@ -802,6 +802,130 @@ testing = ["dev", "providers.tests", "tests_common", "tests", "system", "unit", # mechanism and whole .pyi is really "type-checking" only "providers/common/sql/src/airflow/providers/common/sql/hooks/sql.pyi" = ["TID253"] +# Allow Task SDK and its tests to import from task SDK +"task-sdk/**" = ["TID251"] Review Comment: Nope. I am suggesting we do both. They serve a bit different purposes and are triggered in a different moment. I was deliberating, whether there is a rule in ruff that we could use to expand it and generally fail when there is any import that is not present at all in the venv (but this is something that - probably correctly) `astral` team is going to have in `ty`. One big (and important) difference of `ruff` static checking vs. `mypy` or `ty` static (type) checking - is that `ruff` is able to just look at the sources and figure out if things are right or not - this is for example why we can run `ruff` in pre-commit outside of breeze in local venv, because it generally does not need to install all the 700+ dependencies to do it's job. On the other hand checking if "import" is missing, requires building locally the virtualenv that should have all the dependnecies of things that we check installed - for example if you want to check google provider, you need to have all google provider dependencies installed and their dependencies (for example plyvel that is inherently difficult to install on MacOS for example, or pymsql for mysql provider and it requires to have mysql system-level libraries installed). This is why current `mypy` and future `ty` (if we switch to it) will necessarliy have to be run inside the image or after runnig `uv sync --all-packages` in local env (which is not guaranteed to work outside of the breeze image and relies on a number of 3rd-party system libraries we need to have installed and build tools installed in the system) So I don't think anything is missing, and think just doing what we already doing in "lowest dependency" tests is enough (for now): ``` cd <distribution> # (for example `providers/amazon`) uv sync --resolution lowest-direct pytest tests ``` This effectively does what I wanted to do "implicitly" - while runnig all the test, we assume that all the `src` files will be imported as part of `pytest tests` -> and they will fail if we import from airflow (because eventually "task-sdk" will not depende on "airflow-core" and `uv sync` will simply remove airflow-core imports from PYTHON_PATH. But later with `ty` we can just scan all the sources in similar fashion: ``` cd <distribution> # (for example `providers/amazon`) uv sync --resolution lowest-direct ty src tests ``` And there - such import checks will be done automatically for all sources. So summarizing - no change now - this was a bit of a digression and future work. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
