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 the incompatibility bug of azure deployment id configuration with gpt4/3-only #4098

Merged
merged 32 commits into from
Jul 7, 2023

Conversation

IANTHEREAL
Copy link
Contributor

@IANTHEREAL IANTHEREAL commented May 11, 2023

Background

When I use the azure openai api and set the set gpt4 only, auto gpt will fail because the deployment id is incorrect.

azure.yaml

azure_api_type: azure
azure_api_base: https://test.openai.azure.com/
azure_api_version: 2023-03-15-preview
azure_model_map:
    fast_llm_model_deployment_id: gpt-3-5_playground
    smart_llm_model_deployment_id: gpt-4_playground
    embedding_model_deployment_id: gpt-embedding-ada

min test script

from autogpt.config.config import Config

CFG = Config()
print(CFG.fast_llm_model, CFG.get_azure_deployment_id_for_model(CFG.fast_llm_model))
print(CFG.smart_llm_model, CFG.get_azure_deployment_id_for_model(CFG.smart_llm_model))

CFG.set_smart_llm_model(CFG.fast_llm_model)
print(CFG.fast_llm_model, CFG.get_azure_deployment_id_for_model(CFG.fast_llm_model))
print(CFG.smart_llm_model, CFG.get_azure_deployment_id_for_model(CFG.smart_llm_model))

output

gpt-3.5-turbo -> gpt-3-5_playground
gpt-4 -> gpt-4_playground
gpt-4 -> gpt-3-5_playground
gpt-4 -> gpt-3-5_playground

But it should be

gpt-3.5-turbo -> gpt-3-5_playground
gpt-4 -> gpt-4_playground
gpt-4 -> gpt-4_playground
gpt-4 -> gpt-4_playground

Changes

When the user sets set gpt4 only, modify the corresponding fast llm model related configuration

Documentation

Test Plan

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@vercel
Copy link

vercel bot commented May 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Jun 7, 2023 3:16am

@k-boikov k-boikov added the Azure label May 14, 2023
@k-boikov k-boikov added this to the v0.3.2-release milestone May 14, 2023
@vercel vercel bot temporarily deployed to Preview May 19, 2023 10:55 Inactive
@lc0rp
Copy link
Contributor

lc0rp commented May 19, 2023

The underlying issue is addressed in #3144.

@IANTHEREAL Shall we close this and collaborate there?

@lc0rp lc0rp self-assigned this May 19, 2023
@IANTHEREAL
Copy link
Contributor Author

IANTHEREAL commented May 22, 2023

@lc0rp Hi,but I think these two PRs don't solve the same problem

@vercel vercel bot temporarily deployed to Preview May 22, 2023 19:59 Inactive
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.06 ⚠️

Comparison is base (9e5492b) 50.04% compared to head (6828d11) 49.98%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4098      +/-   ##
==========================================
- Coverage   50.04%   49.98%   -0.06%     
==========================================
  Files         116      116              
  Lines        4826     4825       -1     
  Branches      650      650              
==========================================
- Hits         2415     2412       -3     
- Misses       2227     2228       +1     
- Partials      184      185       +1     
Impacted Files Coverage Δ
autogpt/config/config.py 82.55% <83.33%> (-1.35%) ⬇️
autogpt/configurator.py 29.48% <100.00%> (-0.90%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lc0rp
Copy link
Contributor

lc0rp commented May 23, 2023

@lc0rp Hi, but I think these two PRs don't solve the same problem

Well, not exactly the same, but parts of this PR overlap with #3144 and this PR may be fixed by #3144. So, I should have said they're related. In this PR, you suggest that if we're in gpt4only mode, we should pull settings from the CFG.smart_llm_model and vice-versa.

In PR #3144, we ignore the config when we select an exclusive mode at the command line. It shouldn't matter whether the smart_llm_model is set to 3.5 or 4 - in gpt4only mode, we should use GPT-4.

Copy link
Contributor

@lc0rp lc0rp left a comment

Choose a reason for hiding this comment

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

Added a couple of comments. Happy to hop on a discord chat/call.

autogpt/configurator.py Outdated Show resolved Hide resolved
autogpt/configurator.py Outdated Show resolved Hide resolved
autogpt/configurator.py Outdated Show resolved Hide resolved
autogpt/configurator.py Outdated Show resolved Hide resolved
@lc0rp lc0rp modified the milestones: v0.4.0 Release, v0.4.1 Release May 23, 2023
@IANTHEREAL
Copy link
Contributor Author

@lc0rp Hi
The crux of the issue we're discussing is that, although the model only consists of GPT3.5 and GPT4, there are multiple parameters that can be set independently. Hence, we are forced to make some assumptions based on expectations.

My background is that I'm develop an online service, and neither errors nor performance degradation are acceptable to me.

For example, when setting 'gpt4only':
1.1. Incorrectly selecting the deployment id due to 'gpt4 only' setting shouldn't occur.
1.2. I'd prefer not to see a trade-off where setting the fast model token limit to 4000 results in smaller memory and a subsequent loss of performance.

The same applies when setting 'gpt3only'. Encountering an error due to the smart model token limit being 8000 is something I'd rather not see.

At present, my modifications have only mitigated these issues, but this isn't the best solution. Incorporating these related parameters into two separate GPT configuration objects will be more nature, thus avoiding these discussions that I see as somewhat compromised.

I apologize if my response is a bit rushed. If you have thoughts, I'd be happy to discuss further on the community discord after I finish a project in the next couple of days.

To add to that, I've just tested your code with gpt4only=True, and it seems that the function get_azure_deployment_id_for_model() isn't working as expected.

@IANTHEREAL
Copy link
Contributor Author

IANTHEREAL commented May 25, 2023

Proposal for a Structured Configuration Object for GPT Models

In the current implementation of Auto-GPT, there are multiple parameters related to GPT-3 and GPT-4 models that can be set independently. This can lead to unexpected behavior when the deployment ID for the model is fetched using the get_azure_deployment_id_for_model() function. A solution to this problem is to encapsulate all model-related parameters into a structured configuration object, which will both simplify the code and reduce the likelihood of errors.

Proposal

The proposal is to introduce a GPTConfig data class that contains all relevant parameters for a GPT model. Here is a potential implementation:

@dataclass
class GPTConfig:
    model: str
    api_key: str
    token_limit: int = 4000
    temperature: float = 0
    api_deployment_id: Optional[str] = None

With this data class, we can create separate configuration objects for GPT-3 and GPT-4 models from ENV:

gpt3 = GPTConfig(model='gpt-3', api_key='gpt3_api_key', api_deployment_id='gpt-3_deployment_id')
gpt4 = GPTConfig(model='gpt-4', api_key='gpt4_api_key', api_deployment_id='gpt-4_deployment_id')

To manage these configurations, a Config class can be created:

class Config:
    def __init__(self):
        self.model_config = {
            'gpt-3': gpt3,
            'gpt-4': gpt4,
        }
        self.fast_llm_model = 'gpt-3'
        self.smart_llm_model = 'gpt-3'

    def set_fast_llm_model(self, model: str):
        # check whether model is in self.model_config
        if model in self.model_config:
            self.fast_llm_model = model
        else:
            raise ValueError(f"Model {model} is not in the configuration")

Benefits

  1. Code Simplification: Encapsulating all model-related parameters into a single data structure simplifies the codebase and makes it easier to understand and maintain. This approach brings clarity to the configuration process by making the relationships between the different parameters explicit. This can make the system easier to debug and extend.

  2. Error Reduction: By ensuring that all model-related parameters are set consistently, this approach reduces the likelihood of errors.

  3. Enhanced Flexibility: This approach allows for easy configuration of multiple heterogeneous models in the future. If a new model variant is introduced, a new configuration object can be easily created for it.

@IANTHEREAL
Copy link
Contributor Author

Proposal for a Structured Configuration Object for GPT Models

@lc0rp How do you feel about it? Do you think you could take on the implementation of this proposal? I would really appreciate your assistance with this.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label May 26, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@lc0rp
Copy link
Contributor

lc0rp commented May 27, 2023

Proposal for a Structured Configuration Object for GPT Models

@lc0rp How do you feel about it? Do you think you could take on the implementation of this proposal? I would really appreciate your assistance with this.

Oops, I was absent these past few days. However, I agree with you regarding the matter of multiple independent variables that need to be linked, and I must say, your proposal looks fantastic!

I am pleased to note that the re-architectural design has also reached similar conclusions, and your proposal resembles the re-architectural code. The re-architectural code is situated in a separate branch as all the loose ends have been tied. It is almost ready for release.

Take a look at the LLM base models through this link: https://github.com/Significant-Gravitas/Auto-GPT/blob/agent-state-encapsulation/autogpt/llm/base.py and let me know your thoughts. If you'd like to continue the proposal afterward, I'd be happy to collaborate. We'll move it to a new DRAFT issue and invite feedback before PR submission.

@IANTHEREAL
Copy link
Contributor Author

@lc0rp Similar to how the static attributes of GPT are preserved in the LLM base models: https://github.com/Significant-Gravitas/Auto-GPT/blob/agent-state-encapsulation/autogpt/llm/base.py, I think we can implement a similar GPT dynamic attributes configuration. I'd like to convert this PR into a draft and try to modify it. If you have any other plans or thoughts, please feel free to share with me at any time.

@IANTHEREAL IANTHEREAL marked this pull request as draft May 29, 2023 01:51
@lc0rp lc0rp added this to the v0.4.2 Release milestone Jun 14, 2023
@lc0rp
Copy link
Contributor

lc0rp commented Jun 23, 2023

@IANTHEREAL Is this still valid? If so, we're prepping for release v0.4.3. Please resolve conflicts and stand by as we merge.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Jun 26, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@netlify
Copy link

netlify bot commented Jun 26, 2023

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit f56ff98
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/64a78c7e12b0f100081759f4

@IANTHEREAL
Copy link
Contributor Author

@lc0rp Yes, I have resolved it

@lc0rp lc0rp modified the milestones: v0.4.3 Release, v0.4.4 Release Jun 26, 2023
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Jun 27, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

The config module was overhauled in #4803, causing conflicts with this PR (again). I'm not sure why this PR wasn't included in an earlier release yet, it looks like it was mergeable.

Anyhow, it seems that #4803 also broke support for Azure by removing Config.get_azure_deployment_id_for_model(). Instead of resolving the merge conflicts on this PR, maybe it's easier to make a new PR fixing Azure support altogether in a new PR.

Update: @collijk will look into patching basic Azure support first, and then we can check if the issue addressed by this PR still exists.
Update 2: looks like this PR fixes Azure support: #4875

tests/unit/test_config.py Outdated Show resolved Hide resolved
@Pwuts Pwuts assigned Pwuts and unassigned Pwuts Jul 6, 2023
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Jul 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@Pwuts Pwuts self-assigned this Jul 7, 2023
@Pwuts Pwuts merged commit 3b7e101 into Significant-Gravitas:master Jul 7, 2023
@collijk collijk mentioned this pull request Jul 7, 2023
6 tasks
@IANTHEREAL IANTHEREAL deleted the set-gpt3/4-only branch July 10, 2023 06:50
dayofthedave pushed a commit to dayofthedave/Auto-GPT that referenced this pull request Jul 17, 2023
* Fix --gpt3only and --gpt4only

* Fix and consolidate test_config.py::test_azure_config (x2)

---------

Co-authored-by: Luke K (pr-0f3t) <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Reinier van der Leer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants