-
Notifications
You must be signed in to change notification settings - Fork 160
refactor!: Make the FastAPI and Starlette dependencies optional #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor!: Make the FastAPI and Starlette dependencies optional #217
Conversation
With this update, using A2AFastAPIApplication requires either adding the FastAPI package directly to the project or indicating the "fastapi" group when installing a2a-sdk, e.g., with uv "uv add a2a-sdk[fastapi]" or with pip "pip install a2a-sdk[fastapi]"
When defining missing optional dependencies from fastapi, mypy issues errors of the following form: 'Name "FastAPI" already defined (possibly by an import) [no-redef]'. This commit fixes such mypy errors.
Updates the FastAPI optional dependency installation instructions in README.md.
On app initialization with app = A2aFastAPIApplication(), check that the FastAPI package is installed and raise ImportError with the custom messsage if necessary.
This won't solve the core issue on its own, because starlette is also a dependency and it depends on fastapi, so you'll need to remove that dependency as well. |
Hi @holtskinner,
It seems the dependencies are the other way around, so if we do it one framework at a time, I think refactoring FastAPI first makes sense. Starlette dependency treeA quick dependency check shows that
for reference, see FastAPI dependency tree hereFastAPI dependency treeOn the other hand, if a project requires
SDK development vs downstream project dependenciesI kept FastAPI in dev dependencies so that when working on the SDK itself, i.e., running At the same time, with this refactoring non-FastAPI downstream projects (agents) that use Considerations for next steps (refactoring
|
Package respx was added by another PR, so we need to resync uv.lock.
3788450
to
1d06e63
Compare
…pr/darkhaniop/217
It appears that the unit test task is failing because coverage has dropped below 90%. Running unit tests locally shows that all tests pass with a threshold of 89%.
Should we consider lowering option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff in 16059fb
--- a/src/a2a/server/apps/jsonrpc/fastapi_app.py
+++ b/src/a2a/server/apps/jsonrpc/fastapi_app.py
@@ -58,17 +58,6 @@ class A2AFastAPIApplication(JSONRPCApplication):
context_builder=context_builder,
)
- self._check_fastapi_dependency()
-
- def _check_fastapi_dependency(self) -> None:
- """Checks if the FastAPI package is installed.
-
- If instead of the actual FastAPI class, a dummy implementation from
- ./fastapi_import_helpers.py is imported, initializing FastAPI() would
- raise ImportError.
- """
- _app = FastAPI()
-
def add_routes_to_app(
self,
app: FastAPI,
I think this removed code was a better way to check the installed FastAPI framework vs. how it was grouped with Starlette in a later update in this PR. Because, in the current version, we again lose the capability to use the a2a-sdk
package with only Starlette (as both framework imports are within the same try-except block, which would fail if only one of the frameworks is present).
Also, maybe checking for the presence of a particular framework in the JSONRPCApplication
constructor (which I believe is intended to be framework-agnostic) is not the best approach. For instance, a developer may want to bring another framework and pass it using the context_builder
param.
Update: Looking more into this, because response generation methods defined in JSONRPCApplication
are not abstract but return Starlette responses, it requires both starlette
and sse-starlette
(still it does not, and should not depend on FastAPI).
edit: clarity and add the commit ref with diff
edit2: add more details about JSONRPCApplication
dependencies
Since all three apps A2AStarletteApplication, A2AFastAPIApplication, and JSONRPCApplication are public APIs, we should assume that each can be used direcly by downstream projects. Therefore, proper ImportError messages should be printed when initializing each type of app with missing optional dependencies.
Since all three apps Also, as I explained above, it is better to decouple framework dependencies to allow the initialization of In the latest refactoring, instead of telling the developer that all frameworks should be added with if not _http_server_installed:
raise ImportError(
'The `a2a-sdk[http-server]` package is required to use the `JSONRPCApplication`.'
) I updated it to provide clearer details: if not _package_starlette_installed:
raise ImportError(
'Packages `starlette` and `sse-starlette` are required to use the'
' `JSONRPCApplication`. They can be added as a part of `a2a-sdk`'
' optional dependencies, `a2a-sdk[http-server]`.'
) ChecksI tested this version with a sample agent and verified that it prints the correct message if optional dependencies are missing. Also, I checked if a project adds only P.S. The initial version of this PR proposed to introduce framework-specific dependency groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, can you add new tests for the updated sections?
The added tests check that ImportError with relevant messages are raised when creating instances of A2AStarletteApplication, A2AFastAPIApplication, and JSONRPCApplication with missing optional dependencies.
In future versions of a2a-sdk, HTTP server packages may become optional. See a2aproject/a2a-python/pull/217 for details. This PR ensures that agents in `samples/python/agents/` continue to work with the future versions on a2a-sdk. Refs a2aproject#190
I added some tests. BTW, I did not want to make them "too hacky" to force certain imports to truly fail, so I tested "import failures" by flipping |
fc1ac9a
to
e8233d1
Compare
The last force push of this branch from fc1ac9a to e8233d1 above is to avoid re-adding a notebook file According to this explanation, it was removed from git history to keep the SDK tree lighter.
|
Thanks for this. |
Description
With this update, using A2AFastAPIApplication requires either adding the FastAPI package directly to the project or indicating the "fastapi" group when installing a2a-sdk, e.g., with uv "uv add a2a-sdk[fastapi]" or with pip "pip install a2a-sdk[fastapi]"
Additional Checks
A2AFastAPIApplication
.Release-As: 0.3.0