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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,21 @@
)
openai_credentials = APIKeyCredentials(
id="53c25cb8-e3ee-465c-a4d1-e75a4c899c2a",
provider="llm",
provider="openai",
api_key=SecretStr(settings.secrets.openai_api_key),
title="Use Credits for OpenAI",
expires_at=None,
)
anthropic_credentials = APIKeyCredentials(
id="24e5d942-d9e3-4798-8151-90143ee55629",
provider="llm",
provider="anthropic",
api_key=SecretStr(settings.secrets.anthropic_api_key),
title="Use Credits for Anthropic",
expires_at=None,
)
groq_credentials = APIKeyCredentials(
id="4ec22295-8f97-4dd1-b42b-2c6957a02545",
provider="llm",
provider="groq",
api_key=SecretStr(settings.secrets.groq_api_key),
title="Use Credits for Groq",
expires_at=None,
Expand Down
4 changes: 4 additions & 0 deletions autogpt_platform/backend/backend/blocks/llm.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ def AICredentialsField() -> AICredentials:
description="API key for the LLM provider.",
provider="llm",
Pwuts marked this conversation as resolved.
Show resolved Hide resolved
supported_credential_types={"api_key"},
discriminator="model",
discriminator_mapping={
model.value: model.metadata.provider for model in LlmModel
},
)


Expand Down
4 changes: 4 additions & 0 deletions autogpt_platform/backend/backend/data/model.py
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.

Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ def CredentialsField(
supported_credential_types: set[CT],
required_scopes: set[str] = set(),
*,
discriminator: Optional[str] = None,
discriminator_mapping: Optional[dict[str, Any]] = None,
title: Optional[str] = None,
description: Optional[str] = None,
**kwargs,
Expand All @@ -170,6 +172,8 @@ def CredentialsField(
"credentials_provider": provider,
"credentials_scopes": list(required_scopes) or None, # omit if empty
"credentials_types": list(supported_credential_types),
"discriminator": discriminator,
"discriminator_mapping": discriminator_mapping,
}.items()
if v is not None
}
Expand Down
68 changes: 13 additions & 55 deletions autogpt_platform/frontend/src/components/CustomNode.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ import {
BlockUIType,
BlockCost,
} from "@/lib/autogpt-server-api/types";
import { beautifyString, cn, setNestedProperty } from "@/lib/utils";
import {
beautifyString,
cn,
getValue,
parseKeys,
setNestedProperty,
} from "@/lib/utils";
import { Button } from "@/components/ui/button";
import { Switch } from "@/components/ui/switch";
import { history } from "./history";
Expand All @@ -36,8 +42,6 @@ import * as Separator from "@radix-ui/react-separator";
import * as ContextMenu from "@radix-ui/react-context-menu";
import { DotsVerticalIcon, TrashIcon, CopyIcon } from "@radix-ui/react-icons";

type ParsedKey = { key: string; index?: number };

export type ConnectionData = Array<{
edge_id: string;
source: string;
Expand Down Expand Up @@ -181,7 +185,7 @@ export function CustomNode({
nodeId={id}
propKey={propKey}
propSchema={propSchema}
currentValue={getValue(propKey)}
currentValue={getValue(propKey, data.hardcodedValues)}
connections={data.connections}
handleInputChange={handleInputChange}
handleInputClick={handleInputClick}
Expand All @@ -204,7 +208,7 @@ export function CustomNode({
className=""
selfKey={noteKey}
schema={noteSchema as BlockIOStringSubSchema}
value={getValue(noteKey)}
value={getValue(noteKey, data.hardcodedValues)}
handleInputChange={handleInputChange}
handleInputClick={handleInputClick}
error={data.errors?.[noteKey] ?? ""}
Expand Down Expand Up @@ -240,7 +244,7 @@ export function CustomNode({
nodeId={id}
propKey={propKey}
propSchema={propSchema}
currentValue={getValue(propKey)}
currentValue={getValue(propKey, data.hardcodedValues)}
connections={data.connections}
handleInputChange={handleInputChange}
handleInputClick={handleInputClick}
Expand All @@ -261,11 +265,7 @@ export function CustomNode({
return (
(isRequired || isAdvancedOpen || isConnected || !isAdvanced) && (
<div key={propKey} data-id={`input-handle-${propKey}`}>
{"credentials_provider" in propSchema ? (
<span className="text-m green mb-0 text-gray-900">
Credentials
</span>
) : (
{!("credentials_provider" in propSchema) && (
<NodeHandle
keyName={propKey}
isConnected={isConnected}
Expand All @@ -279,7 +279,7 @@ export function CustomNode({
nodeId={id}
propKey={propKey}
propSchema={propSchema}
currentValue={getValue(propKey)}
currentValue={getValue(propKey, data.hardcodedValues)}
connections={data.connections}
handleInputChange={handleInputChange}
handleInputClick={handleInputClick}
Expand Down Expand Up @@ -336,48 +336,6 @@ export function CustomNode({
setErrors({ ...errors });
};

// Helper function to parse keys with array indices
//TODO move to utils
const parseKeys = (key: string): ParsedKey[] => {
const splits = key.split(/_@_|_#_|_\$_|\./);
const keys: ParsedKey[] = [];
let currentKey: string | null = null;

splits.forEach((split) => {
const isInteger = /^\d+$/.test(split);
if (!isInteger) {
if (currentKey !== null) {
keys.push({ key: currentKey });
}
currentKey = split;
} else {
if (currentKey !== null) {
keys.push({ key: currentKey, index: parseInt(split, 10) });
currentKey = null;
} else {
throw new Error("Invalid key format: array index without a key");
}
}
});

if (currentKey !== null) {
keys.push({ key: currentKey });
}

return keys;
};

const getValue = (key: string) => {
const keys = parseKeys(key);
return keys.reduce((acc, k) => {
if (acc === undefined) return undefined;
if (k.index !== undefined) {
return Array.isArray(acc[k.key]) ? acc[k.key][k.index] : undefined;
}
return acc[k.key];
}, data.hardcodedValues as any);
};

const isHandleConnected = (key: string) => {
return (
data.connections &&
Expand All @@ -400,7 +358,7 @@ export function CustomNode({
const handleInputClick = (key: string) => {
console.debug(`Opening modal for key: ${key}`);
setActiveKey(key);
const value = getValue(key);
const value = getValue(key, data.hardcodedValues);
setInputModalValue(
typeof value === "object" ? JSON.stringify(value, null, 2) : value,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,10 @@ export const CredentialsInput: FC<{
useState<AbortController | null>(null);
const [oAuthError, setOAuthError] = useState<string | null>(null);

if (!credentials) {
if (!credentials || credentials.isLoading) {
return null;
}

if (credentials.isLoading) {
return <div>Loading...</div>;
}

ntindle marked this conversation as resolved.
Show resolved Hide resolved
const {
schema,
provider,
Expand Down Expand Up @@ -226,6 +222,7 @@ export const CredentialsInput: FC<{
if (savedApiKeys.length === 0 && savedOAuthCredentials.length === 0) {
return (
<>
<span className="text-m green mb-0 text-gray-900">Credentials</span>
<div className={cn("flex flex-row space-x-2", className)}>
{supportsOAuth2 && (
<Button onClick={handleOAuthLogin}>
Expand All @@ -246,6 +243,31 @@ export const CredentialsInput: FC<{
)}
</>
);
// Only one saved credential
} else if (
savedApiKeys.length === 0 &&
savedOAuthCredentials.length === 1 &&
selectedCredentials === undefined
) {
if (selectedCredentials === undefined) {
Pwuts marked this conversation as resolved.
Show resolved Hide resolved
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.

}

function handleValueChange(newValue: string) {
Expand All @@ -263,7 +285,7 @@ export const CredentialsInput: FC<{
onSelectCredentials({
id: selectedCreds.id,
type: selectedCreds.type,
provider: schema.credentials_provider,
provider: provider,
// title: customTitle, // TODO: add input for title
});
}
Expand All @@ -272,6 +294,7 @@ export const CredentialsInput: FC<{
// Saved credentials exist
return (
<>
<span className="text-m green mb-0 text-gray-900">Credentials</span>
<Select value={selectedCredentials?.id} onValueChange={handleValueChange}>
<SelectTrigger>
<SelectValue placeholder={schema.placeholder} />
Expand Down
20 changes: 16 additions & 4 deletions autogpt_platform/frontend/src/hooks/useCredentials.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { useContext } from "react";
import { CustomNodeData } from "@/components/CustomNode";
import { BlockIOCredentialsSubSchema } from "@/lib/autogpt-server-api";
import {
BlockIOCredentialsSubSchema,
CredentialsProviderName,
} from "@/lib/autogpt-server-api";
import { Node, useNodeId, useNodesData } from "@xyflow/react";
import {
CredentialsProviderData,
CredentialsProvidersContext,
} from "@/components/integrations/credentials-provider";
import { getValue } from "@/lib/utils";

export type CredentialsData =
| {
Expand Down Expand Up @@ -39,9 +43,17 @@ export default function useCredentials(): CredentialsData | null {
return null;
}

const provider = allProviders
? allProviders[credentialsSchema?.credentials_provider]
: null;
const discriminatorValue: CredentialsProviderName | null =
credentialsSchema.discriminator
? credentialsSchema.discriminator_mapping![
getValue(credentialsSchema.discriminator, data.hardcodedValues)
]
: null;

const providerName =
discriminatorValue || credentialsSchema.credentials_provider;

const provider = allProviders ? allProviders[providerName] : null;

const supportsApiKey =
credentialsSchema.credentials_types.includes("api_key");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,19 @@ export type CredentialsType = "api_key" | "oauth2";

// --8<-- [start:BlockIOCredentialsSubSchema]
export const PROVIDER_NAMES = {
ANTHROPIC: "anthropic",
D_ID: "d_id",
DISCORD: "discord",
GITHUB: "github",
GOOGLE: "google",
GOOGLE_MAPS: "google_maps",
GROQ: "groq",
ntindle marked this conversation as resolved.
Show resolved Hide resolved
IDEOGRAM: "ideogram",
JINA: "jina",
LLM: "llm",
MEDIUM: "medium",
NOTION: "notion",
OLLAMA: "ollama",
OPENAI: "openai",
OPENWEATHERMAP: "openweathermap",
PINECONE: "pinecone",
Expand All @@ -122,6 +125,8 @@ export type BlockIOCredentialsSubSchema = BlockIOSubSchemaMeta & {
credentials_provider: CredentialsProviderName;
credentials_scopes?: string[];
credentials_types: Array<CredentialsType>;
discriminator?: string;
discriminator_mapping?: { [key: string]: CredentialsProviderName };
};

export type BlockIONullSubSchema = BlockIOSubSchemaMeta & {
Expand Down
45 changes: 45 additions & 0 deletions autogpt_platform/frontend/src/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,3 +312,48 @@ export function findNewlyAddedBlockCoordinates(
y: 0,
};
}

type ParsedKey = { key: string; index?: number };

export function parseKeys(key: string): ParsedKey[] {
const splits = key.split(/_@_|_#_|_\$_|\./);
const keys: ParsedKey[] = [];
let currentKey: string | null = null;

splits.forEach((split) => {
const isInteger = /^\d+$/.test(split);
if (!isInteger) {
if (currentKey !== null) {
keys.push({ key: currentKey });
}
currentKey = split;
} else {
if (currentKey !== null) {
keys.push({ key: currentKey, index: parseInt(split, 10) });
currentKey = null;
} else {
throw new Error("Invalid key format: array index without a key");
}
}
});

if (currentKey !== null) {
keys.push({ key: currentKey });
}

return keys;
}
ntindle marked this conversation as resolved.
Show resolved Hide resolved

/**
* Get the value of a nested key in an object, handles arrays and objects.
*/
export function getValue(key: string, value: any) {
const keys = parseKeys(key);
return keys.reduce((acc, k) => {
if (acc === undefined) return undefined;
if (k.index !== undefined) {
return Array.isArray(acc[k.key]) ? acc[k.key][k.index] : undefined;
}
return acc[k.key];
}, value);
}
Loading