Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation #437

mrT23 · 2023-11-07T12:29:07Z

PR Type:

Enhancement

PR Description:

Added support for the 'gpt-4-1106-preview' model with a token limit of 128,000.
Implemented a new function get_max_tokens to dynamically calculate the maximum number of tokens based on model capabilities and configuration limits.
Updated various parts of the code to use the new get_max_tokens function instead of the static MAX_TOKENS dictionary.
Adjusted the configuration.toml to include a new max_model_tokens setting to allow limiting the number of tokens used, regardless of the model's maximum.

PR Main Files Walkthrough:

files:

pr_agent/algo/__init__.py: Added 'gpt-4-1106-preview' model with a token limit of 128,000 to the MAX_TOKENS dictionary.
pr_agent/algo/utils.py: Introduced the get_max_tokens function that returns the maximum number of tokens for a given model, considering the max_model_tokens setting from the configuration.
pr_agent/algo/pr_processing.py: Replaced static MAX_TOKENS dictionary usage with the get_max_tokens function to dynamically calculate token limits during PR processing.
pr_agent/tools/pr_similar_issue.py: Updated to use get_max_tokens instead of MAX_TOKENS for token count checks when processing issues.
pr_agent/settings/configuration.toml: Added a new max_model_tokens setting to allow configuration of a maximum token limit for models.

mrT23 · 2023-11-07T12:29:14Z

/describe

github-actions · 2023-11-07T12:30:18Z

PR Analysis

🎯 Main theme: This PR introduces a new model 'gpt-4-1106-preview' and refactors the way maximum tokens are handled across different models.
📝 PR summary: The PR adds a new model 'gpt-4-1106-preview' to the list of models and its corresponding max tokens. It also refactors the code to handle the maximum tokens allowed by a model. Instead of using a constant, a function 'get_max_tokens' is introduced which takes into account the model's max tokens and a configurable max tokens limit.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 2, because the PR is not very large and the changes are straightforward. However, the impact of the changes is significant and requires careful review.
🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR is well-structured and the changes are logically grouped. However, it would be beneficial to add tests to verify the new functionality. Also, it would be good to add logging in the 'get_max_tokens' function to track when the max tokens are limited by the config value.
🤖 Code feedback:
- relevant file: pr_agent/algo/utils.py
  suggestion: Consider adding logging in the 'get_max_tokens' function to track when the max tokens are limited by the config value. This can be helpful for debugging and understanding the system's behavior. [medium]
  relevant line: '+ return max_tokens_model'
- relevant file: pr_agent/algo/pr_processing.py
  suggestion: It would be beneficial to handle the case when the model is not found in the MAX_TOKENS dictionary in the 'get_max_tokens' function. This can prevent potential errors and make the code more robust. [important]
  relevant line: '+ if total_tokens + OUTPUT_BUFFER_TOKENS_SOFT_THRESHOLD < get_max_tokens(model):'

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

mrT23 · 2023-11-07T12:34:25Z

PR Analysis

🎯 Main theme: Introduction of support for a new GPT-4 model and dynamic token limit calculation.
📝 PR summary: This PR adds support for the 'gpt-4-1106-preview' model with a token limit of 128,000 and implements a function to dynamically calculate token limits. It updates the codebase to use this new function and adds a configuration option to set a maximum token limit.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 3, because the PR involves understanding the token calculation logic and ensuring that the new dynamic token limit calculation is correctly integrated across multiple files.
🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR introduces a significant enhancement by adding support for a new model and improving the flexibility of token limit calculations. It is important to ensure that the new get_max_tokens function is robust and that all parts of the code that previously relied on static token limits are updated accordingly. Additionally, it would be beneficial to include tests to verify the behavior of the new function, especially since token limits can be a critical part of the system's functionality.
🤖 Code feedback:
- relevant file: pr_agent/algo/utils.py
  suggestion: Consider adding a debug log when the token limit is set by the configuration rather than the model's default to aid in troubleshooting and clarity of operation. [medium]
  relevant line: # get_logger().debug(f"limiting max tokens to {max_tokens_model}")
- relevant file: pr_agent/algo/pr_processing.py
  suggestion: It might be beneficial to refactor the repeated pattern of checking token limits against get_max_tokens(model) into a separate function to reduce duplication and improve maintainability. [medium]
  relevant line: if total_tokens + OUTPUT_BUFFER_TOKENS_SOFT_THRESHOLD < get_max_tokens(model):
- relevant file: pr_agent/algo/__init__.py
  suggestion: Ensure that the comment regarding the potential limitation by config.max_model_tokens is accurate and that the code will indeed respect this setting in all cases where the model's token limit is used. [important]
  relevant line: 'gpt-4-1106-preview': 128000, # 128K, but may be limited by config.max_model_tokens
- relevant file: pr_agent/settings/configuration.toml
  suggestion: Validate that the max_model_tokens setting is well-documented and that it's clear to users how this setting interacts with the model-specific token limits. [medium]
  relevant line: max_model_tokens = 32000 # even if a model supports more tokens, quality may degrade. Hence, enable to limit the number of tokens.

pr_agent/algo/__init__.py

mrT23 · 2023-11-07T12:35:46Z

pr_agent/algo/utils.py

+def get_max_tokens(model):
+    max_tokens_model = MAX_TOKENS[model]
+    if get_settings().config.max_model_tokens:
+        if max_tokens_model > get_settings().config.max_model_tokens:
+            max_tokens_model = get_settings().config.max_model_tokens
+            # get_logger().debug(f"limiting max tokens to {max_tokens_model}")
+    return max_tokens_model


Suggestion: Refactor the 'get_max_tokens' function to use a local variable for settings to avoid multiple calls to 'get_settings()' which may be inefficient if it involves I/O operations or complex computations.

Suggested change

def get_max_tokens(model):

max_tokens_model = MAX_TOKENS[model]

if get_settings().config.max_model_tokens:

if max_tokens_model > get_settings().config.max_model_tokens:

max_tokens_model = get_settings().config.max_model_tokens

# get_logger().debug(f"limiting max tokens to {max_tokens_model}")

return max_tokens_model

def get_max_tokens(model):

settings = get_settings()

max_tokens_model = MAX_TOKENS[model]

if settings.config.max_model_tokens and max_tokens_model > settings.config.max_model_tokens:

max_tokens_model = settings.config.max_model_tokens

# get_logger().debug(f"limiting max tokens to {max_tokens_model}")

return max_tokens_model

mrT23 · 2023-11-07T12:35:46Z

pr_agent/algo/utils.py

+def get_max_tokens(model):
+    max_tokens_model = MAX_TOKENS[model]
+    if get_settings().config.max_model_tokens:
+        if max_tokens_model > get_settings().config.max_model_tokens:
+            max_tokens_model = get_settings().config.max_model_tokens
+            # get_logger().debug(f"limiting max tokens to {max_tokens_model}")
+    return max_tokens_model


Suggestion: Add a guard clause to 'get_max_tokens' to handle the case where the model is not found in the MAX_TOKENS dictionary, which would currently raise a KeyError.

Suggested change

def get_max_tokens(model):

max_tokens_model = MAX_TOKENS[model]

if get_settings().config.max_model_tokens:

if max_tokens_model > get_settings().config.max_model_tokens:

max_tokens_model = get_settings().config.max_model_tokens

# get_logger().debug(f"limiting max tokens to {max_tokens_model}")

return max_tokens_model

def get_max_tokens(model):

if model not in MAX_TOKENS:

raise ValueError(f"Model '{model}' not found in MAX_TOKENS dictionary.")

max_tokens_model = MAX_TOKENS[model]

settings = get_settings()

if settings.config.max_model_tokens and max_tokens_model > settings.config.max_model_tokens:

max_tokens_model = settings.config.max_model_tokens

# get_logger().debug(f"limiting max tokens to {max_tokens_model}")

return max_tokens_model

mrT23 · 2023-11-07T12:35:47Z

pr_agent/settings/configuration.toml

@@ -10,6 +10,7 @@ use_repo_settings_file=true
 ai_timeout=180
 max_description_tokens = 500
 max_commits_tokens = 500
+max_model_tokens = 32000 # even if a model supports more tokens, quality may degrade. Hence, enable to limit the number of tokens.


Suggestion: The 'max_model_tokens' setting should be documented to explain its purpose and effect on the system, especially since it overrides the model's default token limit.

Suggested change

max_model_tokens = 32000 # even if a model supports more tokens, quality may degrade. Hence, enable to limit the number of tokens.

# Limits the maximum number of tokens that can be used by any model, regardless of the model's default capabilities.

# This can be useful to ensure consistent performance and to prevent issues related to large token counts.

max_model_tokens = 32000

hussam789 · 2023-11-07T12:38:51Z

pr_agent/algo/utils.py

+        if max_tokens_model > get_settings().config.max_model_tokens:
+            max_tokens_model = get_settings().config.max_model_tokens


Instead, Use the min function:

max_tokens_model = min(max_tokens_model, get_settings().config.max_model_tokens)

mrT23 · 2023-11-07T12:41:53Z

Incremental PR Review

⏮️ Review for commits since previous PR-Agent review: Starting from commit 094f641
(code)

PR Analysis

🎯 Main theme: Introducing support for a new model and dynamic token limit calculation.
📝 PR summary: This PR adds support for the 'gpt-4-1106-preview' model with a specific token limit and implements a function to dynamically calculate the maximum token limit based on model capabilities and configuration settings. It also updates various parts of the code to use this new function and adds a setting to limit tokens in the configuration file.
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
⏱️ Estimated effort to review [1-5]: 3, because the PR involves understanding the new dynamic token calculation logic and ensuring it integrates correctly with existing systems. It also requires verifying that the new configuration setting is properly used across the codebase.
🔒 Security concerns: No

PR Feedback

💡 General suggestions: The PR seems to be well-structured and targets a specific enhancement. It would be beneficial to ensure that there are tests accompanying this new functionality to verify the dynamic token calculation works as expected across different scenarios and model configurations. Additionally, it's important to check that the new configuration setting is documented clearly for end-users.
🤖 Code feedback:
- relevant file: pr_agent/algo/utils.py
  suggestion: Consider caching the settings to avoid multiple calls to get_settings() within the same function or across functions that are frequently called. This can improve performance, especially if get_settings() is not a trivial getter. [medium]
  relevant line: settings = get_settings()
- relevant file: pr_agent/algo/utils.py
  suggestion: It's good practice to log the final token limit after calculation, especially when it's being limited by configuration. Uncomment the debug log line to assist with troubleshooting and provide transparency in the token limiting process. [medium]
  relevant line: # get_logger().debug(f"limiting max tokens to {max_tokens_model}")
- relevant file: pr_agent/settings/configuration.toml
  suggestion: Ensure that the new max_model_tokens setting is documented in the project's configuration guide or README to inform users about its purpose and how to use it. [important]
  relevant line: max_model_tokens = 32000 # Limits the maximum number of tokens that can be used by any model, regardless of the model's default capabilities.

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation

get_max_tokens + added 'gpt-4-1106-preview'

a35a75e

github-actions bot changed the title ~~get_max_tokens + added 'gpt-4-1106-preview'~~ Add support for 'gpt-4-1106-preview' model and implement get_max_tokens function Nov 7, 2023

github-actions bot added the enhancement New feature or request label Nov 7, 2023

mrT23 mentioned this pull request Nov 7, 2023

Switch primary model to GPT-4 Turbo Preview with GPT-4 as fallback #435

Closed

mrT23 requested a review from hussam789 November 7, 2023 12:31

mrT23 changed the title ~~Add support for 'gpt-4-1106-preview' model and implement get_max_tokens function~~ Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation Nov 7, 2023

mrT23 commented Nov 7, 2023

View reviewed changes

code

094f641

hussam789 reviewed Nov 7, 2023

View reviewed changes

hussam789 approved these changes Nov 7, 2023

View reviewed changes

code

54f41dd

mrT23 merged commit 6c82bc9 into main Nov 7, 2023
2 checks passed

mrT23 deleted the tr/new_gpt4 branch November 7, 2023 12:49

yochail pushed a commit to yochail/pr-agent that referenced this pull request Feb 11, 2024

Merge pull request Codium-ai#437 from Codium-ai/tr/new_gpt4

78a0dd9

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation #437

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation #437

mrT23 commented Nov 7, 2023 •

edited

Loading

mrT23 commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

mrT23 commented Nov 7, 2023

mrT23 Nov 7, 2023

mrT23 Nov 7, 2023

mrT23 Nov 7, 2023

hussam789 Nov 7, 2023 •

edited

Loading

mrT23 commented Nov 7, 2023

		if max_tokens_model > get_settings().config.max_model_tokens:
		max_tokens_model = get_settings().config.max_model_tokens

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation #437

Introduce support for 'gpt-4-1106-preview' model and dynamic token limit calculation #437

Conversation

mrT23 commented Nov 7, 2023 • edited Loading

PR Type:

PR Description:

PR Main Files Walkthrough:

mrT23 commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

PR Analysis

PR Feedback

How to use

mrT23 commented Nov 7, 2023

PR Analysis

PR Feedback

mrT23 Nov 7, 2023

Choose a reason for hiding this comment

mrT23 Nov 7, 2023

Choose a reason for hiding this comment

mrT23 Nov 7, 2023

Choose a reason for hiding this comment

hussam789 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

mrT23 commented Nov 7, 2023

Incremental PR Review

PR Analysis

PR Feedback

mrT23 commented Nov 7, 2023 •

edited

Loading

hussam789 Nov 7, 2023 •

edited

Loading