shahar1 commented on code in PR #70791: URL: https://github.com/apache/airflow/pull/70791#discussion_r3694764619
########## providers/common/sql/pyproject.toml: ########## @@ -72,11 +72,11 @@ dependencies = [ # Any change in the dependencies is preserved when the file is regenerated [project.optional-dependencies] "pandas" = [ - 'pandas[sql-other]>=2.1.2; python_version <"3.13"', + 'pandas[sql-other]>=2.1.2,<3; python_version <"3.13"', Review Comment: Every one of the 45 new `<3` specifiers lands without a rationale comment. From [`contributing-docs/13_airflow_dependencies_and_extras.rst`](https://github.com/apache/airflow/blob/main/contributing-docs/13_airflow_dependencies_and_extras.rst): > We very rarely upper-bind dependencies - only when there is a known conflict with a new or upcoming version of a dependency that breaks the installation or tests (and we always make a comment why we are upper-bounding a dependency). [`AGENTS.md`](https://github.com/apache/airflow/blob/main/AGENTS.md) § *Tracking issues for deferred work* additionally wants the tracking-issue URL here in the file rather than only in a PR comment, using the full URL so it resolves when someone is grepping a checkout rather than reading GitHub. ```suggestion # pandas 3 changes the dtypes a DataFrame reads back with, so DataFrame XComs do not # round trip; capped until pandas 3 support lands. # Tracked at https://github.com/apache/airflow/issues/<tracking-issue> 'pandas[sql-other]>=2.1.2,<3; python_version <"3.13"', ``` The same one-liner at each of the other 13 provider sites would make the caps self-explaining for whoever has to decide when they come off. ########## task-sdk/tests/task_sdk/serde/test_serializers.py: ########## @@ -286,6 +286,43 @@ def test_pandas_serializers(self): assert serialize(123) == ("", "", 0, False) + def test_pandas_3_dataframe_name_is_registered(self): + """The pandas 3 qualname must be registered, or the guard below is unreachable. + + Under pandas 3 a DataFrame qualifies as ``pandas.DataFrame``. serde dispatches on + that name, so if it is absent from the registry the caller gets the generic + "cannot serialize object of type" from serde and never reaches this module. + """ + from airflow.sdk.serde.serializers.pandas import deserializers, serializers + + assert "pandas.DataFrame" in serializers + assert "pandas.core.frame.DataFrame" in deserializers + + @pytest.mark.parametrize("version", ["3.0.0", "3.0.5", "4.1.2"]) + def test_pandas_3_dataframe_xcom_is_refused(self, monkeypatch, version): + """pandas >= 3 must fail loudly on both write and read, not round trip silently.""" + import pandas as pd + + from airflow.sdk.serde.serializers import pandas as pandas_serializer + + monkeypatch.setattr(pd, "__version__", version) + df = pd.DataFrame(data={"col1": [1, 2], "col2": [3, 4]}) + + with pytest.raises(RuntimeError, match=rf"DataFrame XComs are not supported with pandas {version}"): + pandas_serializer.serialize(df) + + with pytest.raises(RuntimeError, match=r"DataFrame XComs are not supported with pandas"): + pandas_serializer.deserialize(pd.DataFrame, 1, "") + + @pytest.mark.parametrize("version", ["2.1.2", "2.3.3"]) + def test_pandas_2_dataframe_xcom_still_round_trips(self, monkeypatch, version): Review Comment: This one passes unchanged on `main`. Before this PR nothing reads `pd.__version__`, so monkeypatching it to `2.1.2` / `2.3.3` while the installed pandas is already 2.x changes nothing observable — and the round trip itself is already covered by `test_pandas_serializers` a few lines above. [`AGENTS.md`](https://github.com/apache/airflow/blob/main/AGENTS.md) testing standards: > Every changed or added behaviour must have a test; every test must fail without the PR's change. Worth dropping. The other two new tests are exactly right. -- 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]
