Skip to content

[Vertex AI] Set ModelContent.role to nil in system instructions #14558

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

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

andrewheard
Copy link
Contributor

The role property in ModelContent is now ignored (explicitly set to nil) in system instructions. This aligns with the Vertex AI documentation (and the Developer API). This works around an issue where the role is counted in the prompt token count in countTokens(...) but is not in generateContent(...). No code changes are required for developers.

Note: Although not shown in this PR, system instructions are used in most of the integration tests.

Copy link
Contributor

@gemini-code-assist gemini-code-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.

Hello @andrewheard, 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!

Summary of Changes

This pull request addresses an issue where the role property in ModelContent was being incorrectly counted in the prompt token count when using system instructions with the Developer API backend. To resolve this, I've explicitly set the role to nil for system instructions. This aligns with Vertex AI documentation and avoids token count discrepancies between countTokens and generateContent. The changes primarily affect the GenerativeModel.swift file and include updates to the unit tests in VertexComponentTests.swift to reflect this change.

Highlights

  • Bug Fix: Addresses an issue where the role property in ModelContent was being incorrectly counted in the prompt token count for system instructions.
  • Consistency: Ensures consistency between countTokens and generateContent by setting role to nil in system instructions.
  • Alignment with Documentation: Aligns the code with Vertex AI documentation by explicitly setting the role property to nil in system instructions.

Changelog

  • FirebaseVertexAI/Sources/GenerativeModel.swift
    • Modified the systemInstruction property to set the role to nil within the ModelContent when system instructions are used. This change is implemented to avoid token count discrepancies between countTokens and generateContent when using the Developer API backend. The change occurs on line 84, where the systemInstruction is mapped to a new ModelContent object with a nil role.
  • FirebaseVertexAI/Tests/Unit/VertexComponentTests.swift
    • Updated the unit tests to reflect the change in GenerativeModel.swift. Specifically, the tests now expect the systemInstruction to have a nil role. This change ensures that the tests accurately reflect the expected behavior of the code after the fix. The changes occur on lines 221 and 241, where expectedSystemInstruction is created with role: nil.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.


In Vertex's realm, where models reside,
A role's shadow caused tokens to hide.
With a nil assignment, the balance is kept,
Token counts align, while the system has slept.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-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.

Code Review

This pull request addresses an important issue where the role property in ModelContent was being incorrectly counted in the prompt token count when using the Developer API backend. The solution involves explicitly setting the role to nil in system instructions to avoid token count discrepancies between countTokens and generateContent. The code changes are straightforward and well-documented.

Summary of Findings

  • Redundant Initialization: In the test files, the systemInstruction is initialized with a role, but then immediately overwritten with a ModelContent object that has a nil role. This could be simplified by directly initializing expectedSystemInstruction with the nil role.

Merge Readiness

The code changes effectively address the token counting issue and include necessary test updates. While the redundant initialization in the tests is a minor point, addressing it would further improve code clarity. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. However, I believe that this pull request is ready to be merged after addressing the comments.

@andrewheard
Copy link
Contributor Author

  • Redundant Initialization: In the test files, the systemInstruction is initialized with a role, but then immediately overwritten with a ModelContent object that has a nil role. This could be simplified by directly initializing expectedSystemInstruction with the nil role.

This is intentional to verify that the role is ignored when set.

@andrewheard andrewheard marked this pull request as ready for review March 11, 2025 01:22
@andrewheard andrewheard requested a review from paulb777 March 11, 2025 01:22
@andrewheard andrewheard merged commit c9f1c41 into main Mar 11, 2025
37 checks passed
@andrewheard andrewheard deleted the ah/vertex-sys-instruct-role branch March 11, 2025 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants