feat: add a2a agent card generation with the CLI #3606
Conversation
Summary of ChangesHello @guillaumeblaquiere, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the ADK (Agent Development Kit) by adding a new command-line interface tool, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Response from ADK Triaging Agent Hello @guillaumeblaquiere, thank you for creating this PR! To help us review this PR more effectively, could you please provide the following:
This information will help reviewers to review your PR more efficiently. You can find more details in our contribution guidelines. Thanks! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CLI command, generate_agent_card, for A2A agent card generation. The implementation is well-structured, with good error handling for optional dependencies and for processing multiple agents. The new command is correctly integrated into the existing CLI tool. I've added a few comments, mostly related to code style and minor improvements in the new cli_generate_agent_card.py file. The new test case in test_fast_api.py is also a great addition to ensure the A2A runner factory works as expected.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hi @guillaumeblaquiere , Thank you for your contribution through this pull request! |
ryanaiagent
added
the
request clarification
…add-generate-card-command
|
Hi @guillaumeblaquiere , This PR has merge conflicts that require changes from your end. Could you please rebase your branch with the latest main branch to address these? Once this is complete, please let us know so we can proceed with the review. |
ryanaiagent
added
the
a2a
|
@ryanaiagent Let me know if it's ok now |
|
Hi @guillaumeblaquiere, Thank you for your patience here. I apologize for the delay in getting to this review; I know this has been sitting for a while. This PR has merge conflicts that require changes from your end. Could you please rebase your branch with the latest main branch to address these? Once this is complete, please let us know so we can proceed with the review. |
|
@ryanaiagent No worries, everyone has his own constraint ;-) Conflict resolved. |
|
Hi @guillaumeblaquiere , thank you for rebasing. can you also fix the failing unit tests and formatting errors |
|
@ryanaiagent unit test fixed, formatting also |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CLI command, generate_agent_card, which is a valuable addition for A2A communication setup. The implementation is solid, with good use of Click and handling for optional dependencies. My review focuses on improving the CLI's exit behavior for better scripting, enhancing test practices for isolation and clarity, and seeking clarification on some unrelated but significant changes in the test suite. Overall, this is a great feature addition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CLI command generate_agent_card for A2A communication setup, which is a great addition. The implementation is solid, with good error handling for missing dependencies and for processing individual agents. I've added a few suggestions for improvement, mainly around using more specific types and modern libraries.
However, I have a critical concern about a large number of tests being deleted in tests/unittests/agents/test_remote_a2a_agent.py, which might be a mistake. I've also pointed out some empty test cases in the new test file that should be implemented or removed. Please take a look at the detailed comments.
| @click.option( | ||
| "--port", | ||
| default="8000", | ||
| help="Port for the agent URL (default: 8000)", | ||
| ) |
There was a problem hiding this comment.
The port parameter is currently a string. It would be more robust to define it as an integer. You can achieve this by setting type=int and default=8000 in the @click.option for --port. Remember to also update the type hints for port to int in the generate_agent_card and _generate_agent_card_async function signatures.
| agent_dir = os.path.join(cwd, agent_name) | ||
| agent_json_path = os.path.join(agent_dir, "agent.json") | ||
| with open(agent_json_path, "w", encoding="utf-8") as f: | ||
| json.dump(card_dict, f, indent=2) |
There was a problem hiding this comment.
Consider using pathlib for a more modern and object-oriented way to handle file paths. This would involve adding from pathlib import Path at the top of the file, changing os.getcwd() on line 68 to Path.cwd(), using the / operator for joining paths here, and using Path.write_text() to write the file. This can make the path manipulation code cleaner and more readable.
|
|
||
| # Mock A2A client | ||
| mock_a2a_client = create_autospec(spec=A2AClient, instance=True) | ||
| mock_a2a_client = MagicMock(spec=A2AClient) |
There was a problem hiding this comment.
Could you please provide some context on why create_autospec is being replaced with MagicMock here? create_autospec provides stricter mocking which is generally safer by ensuring the mock has the same interface as the spec.
| def test_generate_agent_card_missing_a2a(runner): | ||
| with patch.dict( | ||
| "sys.modules", {"google.adk.a2a.utils.agent_card_builder": None} | ||
| ): | ||
| # Simulate ImportError by ensuring the module cannot be imported | ||
| with patch( | ||
| "builtins.__import__", | ||
| side_effect=ImportError("No module named 'google.adk.a2a'"), | ||
| ): | ||
| # We need to target the specific import in the function | ||
| # Since it's a local import inside the function, we can mock sys.modules or use side_effect on import | ||
| # However, patching builtins.__import__ is risky and affects everything. | ||
| # A better way is to mock the module in sys.modules to raise ImportError on access or just rely on the fact that if it's not there it fails. | ||
| # But here we want to force failure even if it is installed. | ||
|
|
||
| # Let's try to patch the specific module import path in the function if possible, | ||
| # but since it is inside the function, we can use patch.dict on sys.modules with a mock that raises ImportError when accessed? | ||
| # No, that's for import time. | ||
|
|
||
| # Actually, the easiest way to test the ImportError branch is to mock the import itself. | ||
| # But `from ..a2a.utils.agent_card_builder import AgentCardBuilder` is hard to mock if it exists. | ||
| pass | ||
|
|
||
| # Alternative: Mock the function `_generate_agent_card_async` to raise ImportError? | ||
| # No, the import is INSIDE `_generate_agent_card_async`. | ||
|
|
||
| # Let's use a patch on the module where `_generate_agent_card_async` is defined, | ||
| # but we can't easily patch the import statement itself. | ||
| # We can use `patch.dict(sys.modules, {'google.adk.a2a.utils.agent_card_builder': None})` | ||
| # and ensure the previous import is cleared? | ||
| pass |
There was a problem hiding this comment.
This test case is currently empty. Please either implement the test for the ImportError scenario or remove the empty test function. Leaving empty tests can be confusing.
| def test_generate_agent_card_import_error(runner): | ||
| # We need to mock the import failure. | ||
| # Since the import is inside the function, we can patch `google.adk.cli.cli_generate_agent_card.AgentCardBuilder` | ||
| # but that's not imported at top level. | ||
| # We can try to patch `sys.modules` to hide `google.adk.a2a`. | ||
|
|
||
| with patch.dict( | ||
| "sys.modules", {"google.adk.a2a.utils.agent_card_builder": None} | ||
| ): | ||
| # We also need to ensure it tries to import it. | ||
| # The code does `from ..a2a.utils.agent_card_builder import AgentCardBuilder` | ||
| # This is a relative import. | ||
|
|
||
| # A reliable way to test ImportError inside a function is to mock the module that contains the function | ||
| # and replace the class/function being imported with something that raises ImportError? No. | ||
|
|
||
| # Let's just use `patch` on the target module path if we can resolve it. | ||
| # But it's a local import. | ||
|
|
||
| # Let's try to use `patch.dict` on `sys.modules` and remove the module if it exists. | ||
| # And we need to make sure `google.adk.cli.cli_generate_agent_card` is re-imported or we are running the function fresh? | ||
| # The function `_generate_agent_card_async` imports it every time. | ||
|
|
||
| # If we set `sys.modules['google.adk.a2a.utils.agent_card_builder'] = None`, the import might fail or return None. | ||
| # If it returns None, `from ... import ...` will fail with ImportError or AttributeError. | ||
| pass | ||
|
|
||
| # Actually, let's skip the ImportError test for now as it's tricky with local imports and existing environment. | ||
| # The other tests cover the main logic. |
There was a problem hiding this comment.
This test case is also empty. Please either implement it to test the ImportError scenario or remove it.
|
@ryanaiagent I updated the code according to the Gemini review. Let me know the next steps |
3 participants
This PR introduces a new CLI command, generate_agent_card, to the ADK. This command simplifies the setup for A2A (Agent-to-Agent) communication by automating the generation of Agent Cards. It scans the project directory for available agents and produces the necessary JSON configuration for each one.
Key Features
protocol,host, andportused in the agent's endpoint URL.--create-fileflag to automatically save the generated configuration asagent.jsonwithin each agent's directory.Usage Examples
Print Agent Cards to console:
Generate agent.json files for all agents with a custom host and port: