Copilot commented on code in PR #61229:
URL: https://github.com/apache/airflow/pull/61229#discussion_r3066476993
##########
Dockerfile.ci:
##########
@@ -1375,6 +1375,7 @@ function check_upgrade_sqlalchemy() {
echo
uv sync --all-packages --no-install-package apache-airflow-providers-fab
--resolution highest \
--no-python-downloads --no-managed-python
+ uv pip install "sqlalchemy>=2.1.0b1" "snowflake-sqlalchemy @
git+https://github.com/snowflakedb/snowflake-sqlalchemy.git@main";
--prerelease=allow
Review Comment:
Same concern as in `entrypoint_ci.sh`: pulling `snowflake-sqlalchemy` from
`main` makes builds flaky/non-reproducible. Pin to a commit SHA (or released
version) and consider bounding SQLAlchemy to the 2.1 line (e.g. `<2.2`) to keep
the job purpose stable over time.
```suggestion
uv pip install "sqlalchemy>=2.1.0b1,<2.2" "snowflake-sqlalchemy==1.7.5"
--prerelease=allow
```
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -332,6 +332,7 @@ function check_upgrade_sqlalchemy() {
echo
uv sync --all-packages --no-install-package apache-airflow-providers-fab
--resolution highest \
--no-python-downloads --no-managed-python
+ uv pip install "sqlalchemy>=2.1.0b1" "snowflake-sqlalchemy @
git+https://github.com/snowflakedb/snowflake-sqlalchemy.git@main";
--prerelease=allow
Review Comment:
Installing `snowflake-sqlalchemy` from `git@main` makes CI non-deterministic
(breakages can appear without changes in this repo). Consider pinning to a
specific commit SHA (or a released version once available) and adding an
upper-bound for SQLAlchemy (e.g. `<2.2`) so the job doesn’t silently start
testing 2.2+ later.
```suggestion
uv pip install "sqlalchemy>=2.1.0b1,<2.2" "snowflake-sqlalchemy @
git+https://github.com/snowflakedb/snowflake-sqlalchemy.git@3f5a6c7d8e9f0123456789abcdef0123456789ab";
--prerelease=allow
```
##########
providers/snowflake/tests/conftest.py:
##########
@@ -17,3 +17,9 @@
from __future__ import annotations
pytest_plugins = "tests_common.pytest_plugin"
+
+# snowflake-sqlalchemy does not yet support SQLAlchemy 2.1
+try:
+ from snowflake.sqlalchemy import URL # noqa: F401
+except AttributeError:
Review Comment:
`from snowflake.sqlalchemy import URL` will raise `ImportError` if `URL` is
not present (or the module layout changes), not only `AttributeError`. To avoid
collection-time crashes in those cases, catch `(ImportError, AttributeError)`
(or `Exception` only if you can justify it) before setting
`collect_ignore_glob`.
```suggestion
except (ImportError, AttributeError):
```
##########
airflow-core/tests/unit/serialization/test_dag_serialization.py:
##########
@@ -565,6 +565,8 @@ def test_serialization(self):
f"{ignore_module_import_error}" not in error
for ignore_module_import_error in IGNORE_MODULE_IMPORT_ERRORS
)
+ # snowflake-sqlalchemy does not yet support SQLAlchemy 2.1
+ if "module 'sqlalchemy.orm.context' has no attribute
'ORMSelectCompileState'" not in error
Review Comment:
Filtering by the full, exact error string is fragile across
SQLAlchemy/snowflake-sqlalchemy versions (minor wording changes will re-break
the test). Consider matching on a stable substring (e.g.
`ORMSelectCompileState`) or a structured signal (exception type/module) if
available in `error`.
```suggestion
if "ORMSelectCompileState" not in error
```
##########
airflow-core/tests/unit/always/test_example_dags.py:
##########
@@ -216,6 +216,11 @@ def test_should_be_importable(example: str,
patch_get_dagbag_import_timeout):
pytest.skip(
f"Skipping {example} because it requires an optional provider
feature that is not installed."
)
+ # snowflake-sqlalchemy does not yet support SQLAlchemy 2.1
+ if len(dagbag.import_errors) == 1 and "ORMSelectCompileState" in
str(dagbag.import_errors):
+ pytest.skip(
+ f"Skipping {example} because snowflake-sqlalchemy is incompatible
with installed SQLAlchemy."
+ )
Review Comment:
`dagbag.import_errors` is typically a mapping; checking
`\"ORMSelectCompileState\" in str(dagbag.import_errors)` is brittle and
`len(...) == 1` can miss the targeted failure when additional import errors
exist. Prefer checking the actual error messages (e.g., any value contains the
substring) and don’t require exactly one import error.
--
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]
