Skip to content
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

Fix: Adding BUCKET_URI env variable #1600

Merged

Conversation

inardini
Copy link
Contributor

@inardini inardini commented Jan 6, 2025

Description

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Follow the CONTRIBUTING Guide.
  • You are listed as the author in your notebook or README file.
    • Your account is listed in CODEOWNERS for the file(s).
  • Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
  • Ensure the tests and linter pass (Run nox -s format from the repository root to format).
  • Appropriate docs were updated (if necessary)

@inardini inardini requested a review from a team as a code owner January 6, 2025 08:40
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will provide a full review shortly. In the meantime, here's a quick summary for you and other reviewers to get up to speed:

This pull request, authored by inardini, aims to fix an issue by adding a BUCKET_URI environment variable. The change is implemented across three Jupyter Notebook files:

  • gemini/reasoning-engine/evaluating_crewai_agent_reasoning_engine_customized_template.ipynb
  • gemini/reasoning-engine/evaluating_langchain_agent_reasoning_engine_prebuilt_template.ipynb
  • gemini/reasoning-engine/evaluating_langgraph_agent_reasoning_engine_customized_template.ipynb

Specifically, the change inserts a new line (around line 255 in each file) to define BUCKET_URI based on the existing BUCKET_NAME variable. This ensures that the gsutil mb command uses the correctly formatted URI for bucket creation.

Here's a table summarizing the changes:

File Line Number Change
evaluating_crewai_agent_reasoning_engine_customized_template.ipynb ~255 Added BUCKET_URI = f"gs://{BUCKET_NAME}"
evaluating_langchain_agent_reasoning_engine_prebuilt_template.ipynb ~255 Added BUCKET_URI = f"gs://{BUCKET_NAME}"
evaluating_langgraph_agent_reasoning_engine_customized_template.ipynb ~255 Added BUCKET_URI = f"gs://{BUCKET_NAME}"

The intent seems to be to improve the robustness and clarity of the bucket creation process. I'll confirm this during my full review.


And now, a little haiku to brighten your day:

Code flows like a stream,
Fixes merge, a gentle push,
Bugs swim far away.

Copy link
Contributor

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request introduces a helpful change by defining the BUCKET_URI variable, which improves code clarity and reduces redundancy. The PR description is also comprehensive and adheres to the contributing guidelines. However, there's a minor suggestion regarding bucket creation and a potential improvement for project ID handling.

Copy link
Contributor

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will provide feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:

This pull request, authored by @inardini, aims to fix an issue by adding a BUCKET_URI environment variable. The intent is to improve the clarity and maintainability of the code related to Google Cloud Storage bucket creation.

Here's a breakdown of the changes:

  • Added BUCKET_URI variable: The core change involves adding a line to define BUCKET_URI as f"gs://{BUCKET_NAME}". This variable is then used in the subsequent gsutil mb command to create the bucket.
  • Affected Files:
    • gemini/reasoning-engine/evaluating_crewai_agent_reasoning_engine_customized_template.ipynb (line 255 inserted)
    • gemini/reasoning-engine/evaluating_langchain_agent_reasoning_engine_prebuilt_template.ipynb (line 255 inserted)
    • gemini/reasoning-engine/evaluating_langgraph_agent_reasoning_engine_customized_template.ipynb (line 256 inserted)

The change is straightforward and appears to improve the code's readability by explicitly defining the bucket URI. The change is consistent across the three affected files.


And now, a little haiku to lighten the mood:

Buckets in the cloud,
Code improved, clear and bright,
Review flows smoothly.

Copy link
Contributor

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request adds a BUCKET_URI environment variable, which is a good practice for managing external dependencies. The implementation is straightforward and consistent across the modified notebooks. However, there's a minor improvement that can enhance code clarity and efficiency.

@holtskinner holtskinner merged commit b4f16f7 into GoogleCloudPlatform:main Jan 6, 2025
5 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.

2 participants