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

feat: add tutorial_langgraph_human_in_the_loop.ipynb #1673

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shawn-yang-google
Copy link
Contributor

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)

Fixes #<issue_number_goes_here> 🦕

@shawn-yang-google shawn-yang-google requested a review from a team as a code owner January 30, 2025 02:11
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, submitted by shawn-yang-google, aims to add a new Jupyter Notebook tutorial: tutorial_langgraph_human_in_the_loop.ipynb. The description indicates it's a new feature and includes standard contribution checklist items.

Because the provided patch content is empty, I cannot provide specifics on the content of the notebook itself. I'll need to review the notebook directly to provide a detailed analysis of its content and functionality. This summary is therefore limited to the high-level information provided in the pull request metadata.

Summary of Changes:

  • Addition of a new Jupyter Notebook: tutorial_langgraph_human_in_the_loop.ipynb is added to the repository. The exact content of this notebook is currently unknown without access to the actual file contents.

Intent:

The intent appears to be to create a new tutorial focusing on language graphs and human-in-the-loop aspects. However, without more information from the notebook itself, this is just an inference based on the filename.

Disclaimer: This summary is incomplete due to the lack of patch details. A more comprehensive review will follow once I've examined the notebook's contents.


And now for a little haiku to brighten your day:

Code flows like a stream,
Reviews guide it to the sea,
Perfect, clean, and bright.

Copy link
Contributor

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.

@shawn-yang-google shawn-yang-google changed the title feat: Create tutorial_langgraph_human_in_the_loop.ipynb feat: create tutorial_langgraph_human_in_the_loop.ipynb Jan 30, 2025
@shawn-yang-google shawn-yang-google changed the title feat: create tutorial_langgraph_human_in_the_loop.ipynb feat: add tutorial_langgraph_human_in_the_loop.ipynb Jan 30, 2025
Copy link
Collaborator

@holtskinner holtskinner left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I will add more comments once the current ones are resolved.

@shawn-yang-google
Copy link
Contributor Author

Hi Holt, great thanks to all your comments!

I just resolved all of them in b3e453b and 1422472

Copy link
Member

@koverholt koverholt left a comment

Choose a reason for hiding this comment

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

This looks great 🎉 , thank you for the great examples grouped by HITL feature and for the clean inputs and outputs for each. Just a few small tweaks suggested. Otherwise LGTM.

"outputs": [],
"source": [
"agent = reasoning_engines.LanggraphAgent(\n",
" model=\"gemini-1.5-pro-001\",\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggest changing this to gemini-1.5-pro which aliases to the latest GA version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion! Using gemini-1.5-pro as an alias for the latest GA version is a good practice for maintainability and future-proofing. This ensures the notebook remains compatible with updates. I've updated the code suggestion to reflect this change. I'm referencing Google's best practices for Python code style and clarity in this suggestion.

However, for clarity, it might be beneficial to add a comment explaining this choice in the code itself. This will help future readers understand why this specific model identifier was used.

Suggested change
" model=\"gemini-1.5-pro-001\",\n",
agent = reasoning_engines.LanggraphAgent(
model="gemini-1.5-pro", # Alias to the latest Generally Available Gemini model

"source": [
"remote_agent = reasoning_engines.ReasoningEngine.create(\n",
" reasoning_engines.LanggraphAgent(\n",
" model=\"gemini-1.5-pro-001\",\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggest changing this to gemini-1.5-pro which aliases to the latest GA version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion! Using gemini-1.5-pro as an alias for the latest GA version is a good idea for maintainability and future-proofing. This ensures the notebook will continue to work with future updates to the Gemini models. I've updated the code to reflect this change. I'm referencing Google's Python style guide for consistency and readability in this suggestion.

Suggested change
" model=\"gemini-1.5-pro-001\",\n",
model="gemini-1.5-pro",

Comment on lines +144 to +146
"%pip install --upgrade --user --quiet \\\n",
" \"google-cloud-aiplatform[langchain,reasoningengine]\" --force-reinstall"
]
Copy link
Member

Choose a reason for hiding this comment

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

To be explicit, I typically include libs that are imported in the tool functions, which includes the requests lib in this sample notebook / tool. So I suggest adding requests to the list of libraries that are required.

Comment on lines +868 to +870
" requirements=[\n",
" \"google-cloud-aiplatform[reasoningengine,langchain]\",\n",
" ],\n",
Copy link
Member

Choose a reason for hiding this comment

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

To be explicit, I typically include libs that are imported in the tool functions, which includes the requests lib in this sample notebook / tool. So I suggest adding requests to the list of requirements.

" model_kwargs={\"temperature\": 0, \"max_retries\": 6},\n",
" checkpointer_kwargs=checkpointer_kwargs,\n",
" checkpointer_builder=checkpointer_builder,\n",
" # enable_tracing=True, # https://github.com/Arize-ai/openinference/issues/1119\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggest removing the GitHub link in the comment from this sample notebook unless it gives clear actionable instructions for how to work around any issues that might come up.

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.

3 participants