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(#677): turn settings into a separate page #731

Merged
merged 10 commits into from
Jun 9, 2023

Conversation

Jshen123
Copy link
Contributor

@Jshen123 Jshen123 commented Jun 6, 2023

Resolves #677

Description:

  • created new unstyled input component
  • added config options in new settings page
  • use zustand to manage settings state (including localstorage)
    • settings will now save automatically (without the need to click save)
    • reset function is built (button is not included in this PR)

image

Future TODO:

  • replace useSettings with modelSettingsStore

@vercel
Copy link

vercel bot commented Jun 6, 2023

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

Name Status Preview Comments Updated (UTC)
agent-gpt ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 9, 2023 0:33am
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 9, 2023 0:33am

Comment on lines 2 to 4
export const [GPT_35_TURBO, GPT_4] = ["gpt-3.5-turbo" as const, "gpt-4" as const];
export const GPT_MODEL_NAMES = [GPT_35_TURBO, GPT_4];
export type GPTModelNames = typeof GPT_35_TURBO | typeof GPT_4;
Copy link
Contributor

Choose a reason for hiding this comment

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

export type GPTModelNames = "gpt-3.5-turbo" | "gpt-4" 

I think something like this is a bit standard, then you just use the string literals directly (And typing is handled by Typescript)

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 can do that. I wanted to minimize duplicating "gpt-3.5-turbo"

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it feels a bit weird but just they way typescript land works typically

Copy link
Member

Choose a reason for hiding this comment

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

+1 to Asim's approach :)

I think export type GPTModelNames = "gpt-3.5-turbo" | "gpt-4" is just frankly cleaner

@vercel vercel bot temporarily deployed to Preview – docs June 6, 2023 23:10 Inactive
@awtkns
Copy link
Member

awtkns commented Jun 7, 2023

This is looking good! Like the generics!

@vercel vercel bot temporarily deployed to Preview – docs June 7, 2023 02:57 Inactive
@Jshen123 Jshen123 changed the title feat(#677): add combobox for model names feat(#677): turn settings into a separate page Jun 8, 2023
@vercel vercel bot temporarily deployed to Preview – docs June 8, 2023 01:03 Inactive
</SidebarLayout>
);
};

export default SettingsPage;

export const getStaticProps: GetStaticProps = async ({ locale = "en" }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to include this to get translation working. let me know if there's a better way to handle this

Copy link
Member

Choose a reason for hiding this comment

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

@Cs4K1Sr4C any thoughts here?

@Jshen123 Jshen123 marked this pull request as ready for review June 8, 2023 01:07
Copy link
Member

@awtkns awtkns left a comment

Choose a reason for hiding this comment

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

Looking good, just some minor things. Let me know if you wanna sync

Comment on lines 91 to 93
helpText={`${t("CONTROL_MAXIMUM_OF_TOKENS_DESCRIPTION", {
ns: "settings",
})}`}
Copy link
Member

Choose a reason for hiding this comment

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

Since this already returns a string, we don't need to wrap this in a paramaterized string:

Suggested change
helpText={`${t("CONTROL_MAXIMUM_OF_TOKENS_DESCRIPTION", {
ns: "settings",
})}`}
helpText={t("CONTROL_MAXIMUM_OF_TOKENS_DESCRIPTION", { ns: "settings"})}

})}`}
/>
<Input
label={`${t("LOOP", { ns: "settings" })}`}
Copy link
Member

Choose a reason for hiding this comment

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

see comment below :)

max: 1,
step: 0.01
}}
helpText={`${t("HIGHER_VALUES_MAKE_OUTPUT_MORE_RANDOM", {
Copy link
Member

Choose a reason for hiding this comment

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

see coment below :)


<h1 className="mt-6">Advanced Settings</h1>
<Input
label={`${t("TEMPERATURE", { ns: "settings" })}`}
Copy link
Member

Choose a reason for hiding this comment

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

see comment below :)

})}`}
/>
<Input
label={`${t("TOKENS", { ns: "settings" })}`}
Copy link
Member

Choose a reason for hiding this comment

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

see comment below :)

@@ -0,0 +1,42 @@
import React from "react";
import {useTranslation} from "next-i18next";
Copy link
Member

Choose a reason for hiding this comment

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

This import looks unused, let me know if you need help setting up prettier. Installing the prettier plugin will help with formatting 🎨

{...props.attributes}
/>
</div>
{props.helpText && (<p className="mt-2 text-sm text-gray-500" id={`${props.name}-description`}>
Copy link
Member

Choose a reason for hiding this comment

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

Might be som formatting issues here that prettier will solve

'language' as const, 'customModelName' as const, 'customTemperature' as const, 'customMaxLoops' as const, 'maxTokens' as const
];

export interface ModelSettings {
Copy link
Member

Choose a reason for hiding this comment

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

@Jshen123 can we chat about this one?

Note quite sure of the benefit ohh this compared to:

export type ModelSettings = {
  language?: string;
  customModelName?: string;
  customTemperature?: number;
  customMaxLoops?: number;
  maxTokens?: number;
};

name: "agentgpt-settings-storage",
storage: createJSONStorage(() => localStorage),
partialize: (state) => ({
[SETTINGS_LANGUAGE]: state[SETTINGS_LANGUAGE],
Copy link
Member

Choose a reason for hiding this comment

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

see comment below about model settings


const SettingsPage = () => {
const [language, setLanguage] = useState<Language>(ENGLISH);
const [t] = useTranslation();
Copy link
Member

Choose a reason for hiding this comment

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

If you add the settings namescape here as default it can be removed from the rest of your translations

@vercel vercel bot temporarily deployed to Preview – docs June 8, 2023 02:29 Inactive
@Jshen123
Copy link
Contributor Author

Jshen123 commented Jun 8, 2023

Hi @awtkns @asim-shrestha, what's the preference on how we are passing in props? :)
image

@awtkns
Copy link
Member

awtkns commented Jun 8, 2023

Hi @awtkns @asim-shrestha, what's the preference on how we are passing in props? :) image

@Jshen123 it depends on the context. Option two is probably better here as i think each prop is only used once

@vercel vercel bot temporarily deployed to Preview – docs June 8, 2023 23:48 Inactive
@vercel vercel bot temporarily deployed to Preview – docs June 9, 2023 00:29 Inactive
@@ -25,7 +33,7 @@ const links = [
];

interface Props extends PropsWithChildren {
settings: SettingModel;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

making it optional since it's no longer needed for the new settings component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than the commented change, all changes here are from Prettier

@Jshen123 Jshen123 requested a review from awtkns June 9, 2023 00:33
@vercel vercel bot temporarily deployed to Preview – docs June 9, 2023 00:33 Inactive
@awtkns awtkns merged commit c44b757 into main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

⚙️ Turn settings into a separate page
3 participants