Implement lazy resolution for project #4465
Conversation
|
Response from ADK Triaging Agent Hello @jayy-77, thank you for creating this PR! Could you please provide a testing plan for your changes? This will help reviewers to review your PR more efficiently. For more details, please see the contribution guidelines. Thanks! |
adk-bot
added
the
services
Summary of ChangesHello @jayy-77, 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 significantly enhances the configuration flexibility for Google Cloud project and location settings within Vertex AI services. By implementing a lazy resolution mechanism, these critical parameters no longer need to be strictly defined at initialization. Instead, they can be dynamically resolved at runtime from environment variables or Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![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 lazy resolution for GCP project and location configurations, which is a good improvement. The implementation correctly defers loading this configuration until a service method is called in most cases.
However, I've identified a critical issue in VertexAiMemoryBankService where the add_session_to_memory and add_events_to_memory methods do not perform lazy resolution, which could lead to runtime errors if project/location are not provided at initialization. Since the relevant lines are not part of this pull request's changes, I am unable to add a specific comment there, but this should be addressed to fully complete the lazy resolution implementation.
I've also provided a few suggestions to improve code clarity and reduce duplication in the new _resolve_config methods.
| def _resolve_config(self) -> None: | ||
| """Lazily resolves project and location if not provided at initialization. | ||
|
|
||
| This method is called on first use to resolve GCP configuration from: | ||
| 1. Explicit environment variables (highest priority) | ||
| 2. agents_dir root .env file | ||
| 3. Parent directory .env files (walking upward) | ||
|
|
||
| Raises: | ||
| ValueError: If project or location cannot be resolved. | ||
| """ | ||
| if self._config_resolved: | ||
| return | ||
|
|
||
| import os | ||
|
|
||
| # If both are already set (either at init or via environment), we're done | ||
| if self._project and self._location: | ||
| self._config_resolved = True | ||
| self._express_mode_api_key = get_express_mode_api_key( | ||
| self._project, self._location, self._express_mode_api_key | ||
| ) | ||
| return | ||
|
|
||
| # Try to load from environment and .env files | ||
| if self._agents_dir: | ||
| # Load from agents_dir root | ||
| from ..cli.utils import envs | ||
| envs.load_dotenv_for_agent("", self._agents_dir) | ||
|
|
||
| # Resolve from environment after loading .env | ||
| self._project = self._project or os.environ.get("GOOGLE_CLOUD_PROJECT") | ||
| self._location = self._location or os.environ.get("GOOGLE_CLOUD_LOCATION") | ||
|
|
||
| if not self._project or not self._location: | ||
| error_msg = ( | ||
| "GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION must be set. " | ||
| "You can set them via:\n" | ||
| " 1. Environment variables: GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION\n" | ||
| " 2. .env file in your agents_dir\n" | ||
| " 3. Use full resource name: agentengine://projects/{project}/locations/{location}/reasoningEngines/{id}" | ||
| ) | ||
| raise ValueError(error_msg) | ||
|
|
||
| self._config_resolved = True | ||
| self._express_mode_api_key = get_express_mode_api_key( | ||
| self._project, self._location, self._express_mode_api_key | ||
| ) |
There was a problem hiding this comment.
The _resolve_config method has some repeated logic for setting self._config_resolved and calling get_express_mode_api_key. This can be refactored to be more concise and follow the DRY (Don't Repeat Yourself) principle.
def _resolve_config(self) -> None:
"""Lazily resolves project and location if not provided at initialization.
This method is called on first use to resolve GCP configuration from:
1. Explicit environment variables (highest priority)
2. agents_dir root .env file
3. Parent directory .env files (walking upward)
Raises:
ValueError: If project or location cannot be resolved.
"""
if self._config_resolved:
return
import os
if not (self._project and self._location):
# Try to load from environment and .env files
if self._agents_dir:
# Load from agents_dir root
from ..cli.utils import envs
envs.load_dotenv_for_agent("", self._agents_dir)
# Resolve from environment after loading .env
self._project = self._project or os.environ.get("GOOGLE_CLOUD_PROJECT")
self._location = self._location or os.environ.get("GOOGLE_CLOUD_LOCATION")
if not self._project or not self._location:
error_msg = (
"GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION must be set. "
"You can set them via:\n"
" 1. Environment variables: GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION\n"
" 2. .env file in your agents_dir\n"
" 3. Use full resource name: agentengine://projects/{project}/locations/{location}/reasoningEngines/{id}"
)
raise ValueError(error_msg)
self._config_resolved = True
self._express_mode_api_key = get_express_mode_api_key(
self._project, self._location, self._express_mode_api_key
)| self._project, self._location, self._express_mode_api_key | ||
| ) | ||
|
|
||
| def _get_api_client(self): |
There was a problem hiding this comment.
The return type hint for _get_api_client was removed in this change. Restoring it will improve code clarity and help static analysis tools. The method returns an AsyncClient.
def _get_api_client(self) -> vertexai.AsyncClient:| def _resolve_config(self, app_name: Optional[str] = None) -> None: | ||
| """Lazily resolves project and location if not provided at initialization. | ||
|
|
||
| This method is called on first use to resolve GCP configuration from: | ||
| 1. Explicit environment variables (highest priority) | ||
| 2. Agent-specific .env file (if app_name and agents_dir provided) | ||
| 3. agents_dir root .env file | ||
| 4. Parent directory .env files (walking upward) | ||
|
|
||
| Args: | ||
| app_name: Optional app name to load agent-specific .env files. | ||
|
|
||
| Raises: | ||
| ValueError: If project or location cannot be resolved. | ||
| """ | ||
| if self._config_resolved: | ||
| return | ||
|
|
||
| import os | ||
|
|
||
| # If both are already set (either at init or via environment), we're done | ||
| if self._project and self._location: | ||
| self._config_resolved = True | ||
| self._express_mode_api_key = get_express_mode_api_key( | ||
| self._project, self._location, self._express_mode_api_key | ||
| ) | ||
| return | ||
|
|
||
| # Try to load from environment and .env files | ||
| if self._agents_dir and app_name: | ||
| # Load agent-specific .env | ||
| from ..cli.utils import envs | ||
| envs.load_dotenv_for_agent(app_name, self._agents_dir) | ||
| elif self._agents_dir: | ||
| # Load from agents_dir root | ||
| from ..cli.utils import envs | ||
| envs.load_dotenv_for_agent("", self._agents_dir) | ||
|
|
||
| # Resolve from environment after loading .env | ||
| self._project = self._project or os.environ.get("GOOGLE_CLOUD_PROJECT") | ||
| self._location = self._location or os.environ.get("GOOGLE_CLOUD_LOCATION") | ||
|
|
||
| if not self._project or not self._location: | ||
| error_msg = ( | ||
| "GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION must be set. " | ||
| "You can set them via:\n" | ||
| " 1. Environment variables: GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION\n" | ||
| " 2. .env file in your agent directory\n" | ||
| " 3. .env file in your agents_dir\n" | ||
| " 4. Use full resource name: agentengine://projects/{project}/locations/{location}/reasoningEngines/{id}" | ||
| ) | ||
| raise ValueError(error_msg) | ||
|
|
||
| self._config_resolved = True | ||
| self._express_mode_api_key = get_express_mode_api_key( | ||
| self._project, self._location, self._express_mode_api_key | ||
| ) |
There was a problem hiding this comment.
This _resolve_config method has some repeated logic, similar to the one in VertexAiMemoryBankService. Additionally, the .env loading part can be cleaned up to avoid a repeated import. Refactoring this will improve readability and maintainability.
def _resolve_config(self, app_name: Optional[str] = None) -> None:
"""Lazily resolves project and location if not provided at initialization.
This method is called on first use to resolve GCP configuration from:
1. Explicit environment variables (highest priority)
2. Agent-specific .env file (if app_name and agents_dir provided)
3. agents_dir root .env file
4. Parent directory .env files (walking upward)
Args:
app_name: Optional app name to load agent-specific .env files.
Raises:
ValueError: If project or location cannot be resolved.
"""
if self._config_resolved:
return
import os
if not (self._project and self._location):
# Try to load from environment and .env files
if self._agents_dir:
from ..cli.utils import envs
if app_name:
# Load agent-specific .env
envs.load_dotenv_for_agent(app_name, self._agents_dir)
else:
# Load from agents_dir root
envs.load_dotenv_for_agent("", self._agents_dir)
# Resolve from environment after loading .env
self._project = self._project or os.environ.get("GOOGLE_CLOUD_PROJECT")
self._location = self._location or os.environ.get("GOOGLE_CLOUD_LOCATION")
if not self._project or not self._location:
error_msg = (
"GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION must be set. "
"You can set them via:\n"
" 1. Environment variables: GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_LOCATION\n"
" 2. .env file in your agent directory\n"
" 3. .env file in your agents_dir\n"
" 4. Use full resource name: agentengine://projects/{project}/locations/{location}/reasoningEngines/{id}"
)
raise ValueError(error_msg)
self._config_resolved = True
self._express_mode_api_key = get_express_mode_api_key(
self._project, self._location, self._express_mode_api_key
)2 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
_load_gcp_config()loads .env from agents_dir (cwd), not agent directory, so agentengine short IDs fail to resolve project/location #43362. Or, if no issue exists, describe the change:
If applicable, please follow the issue templates to provide as much detail as
possible.
Problem:
A clear and concise description of what the problem is.
Solution:
A clear and concise description of what you want to happen and why you choose
this solution.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required
for all PRs that are not small documentation or typo fixes.
Unit Tests:
Please include a summary of passed
pytestresults.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any
necessary setup or configuration. Please provide logs or screenshots to help
reviewers better understand the fix.
Checklist
Additional context
Add any other context or screenshots about the feature request here.