kaxil commented on code in PR #74449: URL: https://github.com/apache/airflow/pull/74449#discussion_r4218074303
########## providers/common/ai/tests/unit/common/ai/toolsets/test_init.py: ########## @@ -0,0 +1,41 @@ +# 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 pytest + +from airflow.providers.common.ai import toolsets + + [email protected]( + ("name", "module"), + [ + ("DataFusionToolset", "datafusion"), + ("AgentSkillsToolset", "skills"), + ("LoggingToolset", "logging"), + ], +) +def test_toolset_is_exported_from_the_package(name, module): Review Comment: Nothing here covers the missing-DataFusion path the PR describes. `test_missing_langchain_raises_optional_feature_exception` in `test_langchain_bridge.py` is the pattern, but it needs `monkeypatch.delitem` on the cached `airflow.providers.common.ai.toolsets.datafusion` and `airflow.providers.common.sql.datafusion.engine` entries before setting `sys.modules["datafusion"] = None`, otherwise the cached module is returned and the test passes without exercising anything. ########## providers/common/ai/src/airflow/providers/common/ai/toolsets/__init__.py: ########## @@ -61,4 +64,20 @@ def __getattr__(name: str): raise AirflowOptionalProviderFeatureException(e) return MCPToolset + if name == "DataFusionToolset": + try: + from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset + except ImportError as e: + from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException + + raise AirflowOptionalProviderFeatureException(e) + return DataFusionToolset + if name == "AgentSkillsToolset": Review Comment: `skills.py` and `logging.py` only import the stdlib, `pydantic_ai` and provider-internal modules that `HookToolset` already pulls in, so there's nothing optional to defer. Could these two be eager imports next to `HookToolset`? Going through module `__getattr__` makes mypy see both as `Any` from the package root, while the eager import keeps the real signatures. ########## providers/common/ai/src/airflow/providers/common/ai/toolsets/__init__.py: ########## @@ -27,8 +27,11 @@ from airflow.providers.common.ai.toolsets.sandbox import SandboxToolset __all__ = [ + "AgentSkillsToolset", Review Comment: `docs/toolsets/index.rst` (the "Importing toolsets" section) still says six toolsets import from the package root and tells users to import `DataFusionToolset`, `LoggingToolset` and `AgentSkillsToolset` from their submodules. Can you update it in this PR? Something like: every toolset imports from the root, and `SQLToolset`, `MCPToolset` and `DataFusionToolset` raise `AirflowOptionalProviderFeatureException` on first access when their extra isn't installed. ########## providers/common/ai/src/airflow/providers/common/ai/toolsets/__init__.py: ########## @@ -61,4 +64,20 @@ def __getattr__(name: str): raise AirflowOptionalProviderFeatureException(e) return MCPToolset + if name == "DataFusionToolset": + try: + from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset + except ImportError as e: Review Comment: I don't think this `except` can fire. `toolsets/datafusion.py` already wraps its optional imports and raises `AirflowOptionalProviderFeatureException` itself, and that subclasses `AirflowException`, not `ImportError`, so it passes straight through here. A bare import would behave the same (the `SQLToolset` branch above has the same redundancy, so fine to leave if you prefer matching it). ########## providers/common/ai/tests/unit/common/ai/toolsets/test_init.py: ########## @@ -0,0 +1,41 @@ +# 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 pytest + +from airflow.providers.common.ai import toolsets + + [email protected]( + ("name", "module"), + [ + ("DataFusionToolset", "datafusion"), + ("AgentSkillsToolset", "skills"), + ("LoggingToolset", "logging"), + ], +) +def test_toolset_is_exported_from_the_package(name, module): + assert name in toolsets.__all__ + exported = getattr(toolsets, name) + assert exported.__module__ == f"airflow.providers.common.ai.toolsets.{module}" + assert exported.__name__ == name + + +def test_every_name_in_all_resolves(): + for name in toolsets.__all__: + assert getattr(toolsets, name) is not None Review Comment: `getattr` either returns or raises, so `is not None` never decides anything, and the loop stops at the first broken name. `@pytest.mark.parametrize("name", toolsets.__all__)` with a plain `getattr(toolsets, name)` would report each name separately. -- 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]
