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(platform): Simplify Credentials UX #8524

Merged
merged 16 commits into from
Nov 12, 2024

Conversation

kcze
Copy link
Contributor

@kcze kcze commented Nov 1, 2024

Current credentials system always forces users to choose correct credentials, even when they are provided by the cloud platform and should be chosen automatically (e.g. there's only one possible choice).

Changes 🏗️

  • Change provider of default credentials to actual provider names (e.g. anthropic), remove llm provider
  • Add discriminator and discriminator_mapping to CredentialsField that allows to filter credentials input to only allow providers for matching models in useCredentials hook (thanks @ntindle for the idea!); e.g. user chooses GPT4_TURBO so then only OpenAI credentials are allowed
  • Choose credentials automatically and hide credentials input on the node completely if there's only one possible option
  • Move getValue and parseKeys to utils
  • Add ANTHROPIC, GROQ and OLLAMA to providers in frontend types.ts
  • Add hidden field to credentials that is used for default system keys to hide them in user profile
  • Now provider field in CredentialsField can accept multiple providers as a list

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

@kcze kcze requested a review from a team as a code owner November 1, 2024 16:30
@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end platform/blocks size/l labels Nov 1, 2024
@ntindle
Copy link
Member

ntindle commented Nov 4, 2024

This is exactly what I was thinking! great job!

@ntindle
Copy link
Member

ntindle commented Nov 5, 2024

I think the devx and ux of this leaves something to be desired still. I think the dev specifying the valid providers as a list would be useful and then it returns one of the credentials depending on the selected API key added.

Screen.Recording.2024-11-04.at.9.04.20.PM.mov

Also not sure how they are added to the db and filtered but I'm not seeing them after creation, and crashing anytime they're rendered. I think there's an issue somewhere. lmk if you can't replicate

Copy link
Member

@ntindle ntindle left a comment

Choose a reason for hiding this comment

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

Tested, needs addtional work

@kcze
Copy link
Contributor Author

kcze commented Nov 5, 2024

I think the dev specifying the valid providers as a list would be useful and then it returns one of the credentials depending on the selected API key added.

Constraining providers already happens in the backend by specifying discriminator_mapping. You cannot add non-matching provider and even if you manage they won't be displayed in the dropdown.

edit: it's changed so all possible providers need to be set

Regarding bugs and video: everything should now work, including system keys being hidden in the profile.

@kcze kcze requested a review from ntindle November 5, 2024 14:21
Copy link
Member

@ntindle ntindle left a comment

Choose a reason for hiding this comment

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

This still isn't wokring for me
image
There's nowhere to select credentials, and we're still creating llm providers which we shouldn't need

autogpt_platform/backend/backend/blocks/llm.py Outdated Show resolved Hide resolved
autogpt_platform/frontend/src/app/profile/page.tsx Outdated Show resolved Hide resolved
Comment on lines 259 to 283
// Only one saved credential
} else if (
savedApiKeys.length === 0 &&
savedOAuthCredentials.length === 1 &&
selectedCredentials === undefined
) {
if (selectedCredentials === undefined) {
onSelectCredentials({
id: savedOAuthCredentials[0].id,
type: "oauth2",
provider,
title: savedOAuthCredentials[0].title,
});
}
return null;
} else if (savedApiKeys.length === 1 && savedOAuthCredentials.length === 0) {
if (selectedCredentials === undefined) {
onSelectCredentials({
id: savedApiKeys[0].id,
type: "api_key",
provider,
title: savedApiKeys[0].title,
});
}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this? This whole chain of logic seems questionably necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Reading through this PR, I don't really see why this shortcut is being taken. What's the big hurdle preventing the regular credentials input from working on a multi-provider block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand @Pwuts. The purpose of this code is to choose the only one existing credentials and then not show dropdown at all. I simplified this code.

Copy link
Member

@Pwuts Pwuts Nov 12, 2024

Choose a reason for hiding this comment

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

My question is: would it break anything to allow users the choice of adding their own API key? I understand this will be resolved in a follow-up PR, but I don't immediately see what would break right now if you still showed the credentials picker in all cases.

@ntindle
Copy link
Member

ntindle commented Nov 5, 2024

app-index.tsx:25 
 Warning: Cannot update a component (`BatchProvider`) while rendering a different component (`CredentialsInput`). To locate the bad setState() call inside `CredentialsInput`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render Error Component Stack
    at CredentialsInput (credentials-input.tsx:87:9)
    at div (<anonymous>)
    at NodeCredentialsInput (node-input-components.tsx:309:9)
    at NodeGenericInputField (

maybe this error helps

@kcze kcze requested a review from ntindle November 7, 2024 14:30
Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 9f0e386
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/67337246ebbef30008fb3529

@kcze kcze requested review from Pwuts and ntindle November 11, 2024 10:29
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Nov 12, 2024
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

Choose a reason for hiding this comment

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

Just a note: this really has to be considered a tempfix. Scopes are provider-specific. So is supported_credential_types. The CredentialsField was made to handle credentials from a specific provider. If a block supports multiple providers, the discrimination logic should be handled outside the CredentialsField.

Copy link
Member

Choose a reason for hiding this comment

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

See also #8524 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

(I'd like to leave this discussion here for reference in the follow-up PR)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'm happy for us to merge this as a much needed temp hotfix and we can adjust that in a follow up.

@Torantulino Torantulino enabled auto-merge (squash) November 12, 2024 12:52
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Nov 12, 2024
Copy link
Contributor

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

@Pwuts Pwuts disabled auto-merge November 12, 2024 15:23
@Pwuts Pwuts merged commit e907ffd into dev Nov 12, 2024
17 checks passed
@Pwuts Pwuts deleted the kpczerwinski/open-1999-credentials-system-ux-improvements branch November 12, 2024 15:55
Pwuts added a commit that referenced this pull request Nov 13, 2024
Pwuts added a commit that referenced this pull request Nov 15, 2024
…rovider "llm"(#8674)

In #8524, the "llm" credentials provider was replaced. There are still entries with 	"provider": "llm"	 in the system though, and those break if not migrated.

- SQL migration to fix the obvious ones where we know the provider from `credentials.id`
- Non-SQL migration to fix the rest
Pwuts added a commit that referenced this pull request Nov 18, 2024
…rovider "llm" (#8674)

In #8524, the "llm" credentials provider was replaced. There are still entries with `"provider": "llm"` in the system though, and those break if not migrated.

- SQL migration to fix the obvious ones where we know the provider from `credentials.id`
- Non-SQL migration to fix the rest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/blocks platform/frontend AutoGPT Platform - Front end size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants