amoghrajesh commented on code in PR #73116: URL: https://github.com/apache/airflow/pull/73116#discussion_r4056553777
########## devel-common/tests/unit/tests_common/test_utils/test_in_process_taskrun.py: ########## @@ -0,0 +1,44 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import httpx +import httpx2 Review Comment: `devel-common/pyproject.toml` declares neither? Works here because the venv installs multiple libraries where these dependencies are met. I feel like we should add `importorskip` with `httpx2` ########## task-sdk/src/airflow/sdk/api/client.py: ########## @@ -1336,18 +1336,19 @@ def __repr__(self): return repr(self.detail) -class ServerResponseError(httpx.HTTPStatusError): - def __init__(self, message: str, *, request: httpx.Request, response: httpx.Response): +class ServerResponseError(httpx2.HTTPStatusError): Review Comment: `ServerResponseError` is public API. Anyone catching `httpx.HTTPStatusError` or `httpx.RequestError` around an SDK call stops catching it, silently. Anyone passing `httpx.MockTransport` into `Client(transport=...)` gets a stream type failure too. We should support the compat here. ########## airflow-core/src/airflow/api_fastapi/execution_api/app.py: ########## @@ -422,9 +422,11 @@ async def always_allow(request: Request): return self._app + # httpx2, not httpx: the only consumers are Task SDK clients, which subclass httpx2.Client, + # and transports do not cross the httpx/httpx2 package boundary. Review Comment: ```suggestion # httpx2, not httpx: the only consumers are Task SDK clients, which subclass httpx2.Client. ``` ########## devel-common/src/tests_common/test_utils/in_process_taskrun.py: ########## @@ -55,27 +59,44 @@ _XCOM_PATH_PARTS = 5 # /xcoms/{dag_id}/{run_id}/{task_id}/{key} +def resolve_sdk_httpx() -> ModuleType: Review Comment: Does this have to be public API? Usage is within the same module. ########## devel-common/src/tests_common/test_utils/in_process_taskrun.py: ########## @@ -55,27 +59,44 @@ _XCOM_PATH_PARTS = 5 # /xcoms/{dag_id}/{run_id}/{task_id}/{key} +def resolve_sdk_httpx() -> ModuleType: + """Return the httpx implementation the *installed* Task SDK ``Client`` is built on. + + Compat tests run this helper against released Airflow versions whose Task SDK is still + on ``httpx``, while the current one is on ``httpx2``. Transports and responses do not + cross that boundary — httpx asserts on its own stream types — so the transport MUST be + built from the same package the client subclasses. + """ Review Comment: ```suggestion """Return the httpx package the *installed* Task SDK ``Client`` subclasses. Compat jobs pair this helper with a released SDK still on ``httpx``. A transport from the wrong package trips httpx's own stream-type assertion. """ ``` ########## shared/logging/src/airflow_shared/logging/structlog.py: ########## @@ -420,22 +420,16 @@ def structlog_processors( # structlog to ignore. import contextlib + import importlib import click suppress: tuple[ModuleType, ...] = (click, contextlib) - try: - import httpcore - - suppress = (*suppress, httpcore) - except ImportError: - pass - try: - import httpx - - suppress = (*suppress, httpx) - except ImportError: - pass + # airflow-core is still on httpx while the Task SDK is on httpx2, so either (or both) may + # be installed; tracked at https://github.com/apache/airflow/issues/70522 + for module_name in ("httpcore", "httpcore2", "httpx", "httpx2"): Review Comment: The new suppress entries and the new` "httpx2": {"level": "WARN"}` log-level entry have no test. -- 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]
