Skip to content

Conversation

rajeshkanaka
Copy link
Contributor

…bles

  • Filter out None and empty string values from env_vars dictionary
  • Add logging for skipped environment variables
  • Maintain backward compatibility with empty strings in .env files
  • Fixes TypeError when BQML_RAG_CORPUS_NAME and CODE_INTERPRETER_EXTENSION_NAME are empty

Fixes #192

@Copilot Copilot AI review requested due to automatic review settings July 22, 2025 17:58
@rajeshkanaka rajeshkanaka requested a review from 5Y5TEM as a code owner July 22, 2025 17:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a deployment failure in the data-science agent caused by None environment variables being passed to the agent engine. The fix filters out None and empty string values from environment variables while maintaining backward compatibility.

  • Implements filtering logic to exclude None, empty, and whitespace-only environment variables
  • Adds logging for better visibility into which environment variables are being skipped or included
  • Includes code formatting improvements for consistency

@rajeshkanaka rajeshkanaka force-pushed the copilot/vscode1753205589899 branch from 1c013d3 to e30ca22 Compare July 22, 2025 18:07
@mstyer-google mstyer-google self-assigned this Jul 29, 2025
@JuanVargas
Copy link

Yes, please, fix the cod einthe DS project so that it can work well with env variables. It is quite easy, just add somethihng like

model=os.getenv("ROOT_AGENT_MODEL", "gemini-2.5-flash")

@rajeshkanaka
Copy link
Contributor Author

This pull request primarily improves code readability and robustness in the deploy.py deployment script by refactoring environment variable handling, consolidating logging statements, and simplifying function formatting. The most significant update is the new logic for collecting and validating environment variables, which now filters out unset or empty values and logs the process.

Environment variable handling:

  • Refactored the collection of environment variables in main to use a loop over a list of keys, only including variables that are set and non-empty, and logging skipped variables for better transparency.

Logging and error handling:

  • Consolidated multi-line logging statements into single lines for improved readability throughout the file. [1] [2] [3]
  • Added more informative logging when skipping environment variables due to empty or whitespace values.

Code formatting and style:

  • Simplified function signatures and argument formatting to be more concise (e.g., setup_staging_bucket). [1] [2] [3]
  • Cleaned up minor whitespace and formatting issues at the end of the file.

Cloud resource setup:

  • No functional changes, but streamlined the logic for creating and configuring the staging bucket, and clarified related logging.

These changes collectively make the deployment script easier to maintain, less error-prone, and more transparent during execution.

@rajeshkanaka
Copy link
Contributor Author

Dear @5Y5TEM,

I wanted to kindly remind you to review this pull request at your earliest convenience. Please let me know if you have any questions or require additional information from my side.

Thank you for your attention and support.

rajeshkanaka added a commit to rajeshkanaka/adk-samples that referenced this pull request Sep 13, 2025
- Aggregate skipped env var logging
- Clarify comment on None/empty/whitespace
- Enforce 80-char line limit
- Raise UsageError on missing project_id
- Minor formatting cleanups
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

python/agents/data-science/deployment/deploy.py:160

  • Line 159 is missing a space between 'Agent' and the resource_id variable, and line 160 duplicates the same error message. Remove the malformed line 159.
        print(f"\nAgent{resource_id} not found.")
        print(f"\nAgent not found: {resource_id}")

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rajeshkanaka
Copy link
Contributor Author

Hi @mstyer-google @5Y5TEM — followed up on the lingering feedback:

  • Default the data-science root agent to gemini-2.5-flash when ROOT_AGENT_MODEL is unset.
  • Removed the malformed duplicate "Agent not found" print in the deployment script.

Let me know if anything else looks off. Thanks for another look!

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rajeshkanaka rajeshkanaka force-pushed the copilot/vscode1753205589899 branch from a03aa66 to bd88e1a Compare September 19, 2025 20:23
@rajeshkanaka
Copy link
Contributor Author

Yes, please, fix the cod einthe DS project so that it can work well with env variables. It is quite easy, just add somethihng like

model=os.getenv("ROOT_AGENT_MODEL", "gemini-2.5-flash")

This is Fixed now , please review.

Copy link
Contributor Author

@rajeshkanaka rajeshkanaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please let me know if i am still missing some changes.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rajeshkanaka rajeshkanaka force-pushed the copilot/vscode1753205589899 branch 2 times, most recently from 385bf5e to 17b4020 Compare September 29, 2025 20:05
@rajeshkanaka rajeshkanaka force-pushed the copilot/vscode1753205589899 branch from 17b4020 to 61e5bb7 Compare September 29, 2025 20:09
@mstyer-google mstyer-google merged commit 66d7afb into google:main Oct 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deployment fails with None value in .env for BQML_RAG_CORPUS_NAME and CODE_INTERPRETER_EXTENSION_NAME

3 participants