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

Feature/instruct templates #56

Merged
merged 30 commits into from
May 20, 2024
Merged

Conversation

neCo2
Copy link
Contributor

@neCo2 neCo2 commented May 1, 2024

I've added functionality to select from a list of instruct formats. Then you can either click a button to insert the template directly into your prompt, or you can use placeholders, {sys} {/sys} {inst} and {/inst} in your prompt instead, which will be replaced with the selected instruct formats when being evaluated by the model. These placeholders can be used in the persistent context areas as well.
Furthermore, you can add, delete, import and export templates. The UI's ugly as sin, so I'll be more than happy to implement any suggestions you can think of.
firefox_2024-05-01_21-37-11
firefox_2024-05-01_19-49-38
firefox_2024-05-01_20-19-06

I've never worked with indexedDB before, so be sure to double check the code related to this. Judging by some situations I've seen early on, this has potential to lock up the entire page if the db version upgrade doesn't work as intended, until you either clear the storage, or downgrade mikupad.

mikupad.html Show resolved Hide resolved
@neCo2
Copy link
Contributor Author

neCo2 commented May 7, 2024

Having used this for a week now, I'm not sure whether the selected template should be persistent or tied to session. Currently, it's persistent. What do you think?

@lmg-anon
Copy link
Owner

Sorry for my lateness, but this looks VERY solid!

Having used this for a week now, I'm not sure whether the selected template should be persistent or tied to session. Currently, it's persistent. What do you think?

In my opinion, it makes complete sense for the templates to be persistent. In fact, being tied to the session is an issue with how World Info works right now.

Anyway, I'm not very good with UIs either, but here are some of my thoughts for improvements:

  • These icons look quite out of place, they are often associated with "Command Prompt" and "information," respectively. However, I think your idea of using icons was good, and I have no clue what else could fit better... So I guess it's okay to keep these icons if no one gets a better idea.
    image

  • This button should probably be called "Re-Set Defaults" or "Re-Add Defaults".
    image

  • It's a bit unclear what the "Import"/"Export" buttons actually do because they are on the same line as the currently selected template (Also, the Export button tooltip is wrong). I think it would be better to show a clear separation, like below:
    image

  • Creating a new prompt template isn't very obvious because the name change only reflects after you re-open the modal.
    image

@neCo2
Copy link
Contributor Author

neCo2 commented May 19, 2024

Put the import/export stuff on a separate line and fixed the name and tooltip.

they are often associated with "Command Prompt" and "information," respectively

I know, but it's the best I could come up with apart from just an "S" and "I".

name change only reflects after you re-open the modal

Frankly, I can't figure out how to get the name to live-update without breaking stuff.
I made it so that it makes an entry named "New Template" now instead of a completely blank entry. Maybe that helps a little.

In my opinion, it makes complete sense for the templates to be persistent. In fact, being tied to the session is an issue with how World Info works right now.

Just to be clear, I meant the selectedTemplate variable, not the actual templates object.
Also what's the issue with world info being bound to sessions? I guess it's not ideal when you have an entry you want to use in multiple sessions, but could be mostly mitigated with import/export functions, which are probably way overdue anyways.

@neCo2
Copy link
Contributor Author

neCo2 commented May 19, 2024

Added a chat mode similar to what #61 implemented, but working with the instruct templates. You can click the indicated Icon to switch between Completion and Chat mode.
firefox_2024-05-19_14-30-05
Again unsure whether chat mode should be persistent or session-based.

@lmg-anon
Copy link
Owner

Frankly, I can't figure out how to get the name to live-update without breaking stuff.
I made it so that it makes an entry named "New Template" now instead of a completely blank entry. Maybe that helps a little.

Yes, that is better, but I think you could implement live-updating like this. I just tested it here and it seems to work fine:

  • Add a new state to keep track of the new name (const [newTemplateName, setNewTemplateName] = useState(undefined);)
  • Update the state in handleInstructTemplateChange together with nameNew
  • Undefine the state in updateTemplateDB
  • Set the value in SelectBoxTemplate to be newTemplateName ?? selectedTemplate instead of selectedTemplate
  • Change the code in SelectBoxTemplate to use o.nameNew instead of o.value/o.name

Just to be clear, I meant the selectedTemplate variable, not the actual templates object.

Oh yeah, selectedTemplate makes sense to be tied to the session, just like everything else in the "Parameters" group.

Also what's the issue with world info being bound to sessions? I guess it's not ideal when you have an entry you want to use in multiple sessions, but could be mostly mitigated with import/export functions, which are probably way overdue anyways.

Yes, it's just to be able to easily share with other sessions and avoid duplication of data. One could have a list of different World Infos, and easily switch between them. But I agree, import/export functions would mitigate this nicely.

Added a chat mode similar to what #61 implemented, but working with the instruct templates. You can click the indicated Icon to switch between Completion and Chat mode. Again unsure whether chat mode should be persistent or session-based.

This works pretty nicely, damn. I think this could also be tied to the session.

@neCo2
Copy link
Contributor Author

neCo2 commented May 19, 2024

That works great! Added the live-updating just like you suggested, and tied selectedTemplate and chatMode to session.
And now that the title's live-updating, I also changed back to a fully empty entry for adding a new template.

One could have a list of different World Infos, and easily switch between them

That's a great idea. It should be possible to copy a lot of the code of this PR to save and update WI to the indexedDB. Maybe I'll look into that.

mikupad.html Outdated Show resolved Hide resolved
mikupad.html Outdated Show resolved Hide resolved
mikupad.html Outdated Show resolved Hide resolved
mikupad.html Outdated Show resolved Hide resolved
mikupad.html Outdated Show resolved Hide resolved
@neCo2
Copy link
Contributor Author

neCo2 commented May 20, 2024

Changed the indicated useMemos to useEffect and moved updateTemplateList inside the instruct modal.
As I already said, I'd really appreciate it if you could handle the indexedDB stuff. Also I absolutely do not mind if you delay merging this PR until that is addressed.

@neCo2 neCo2 requested a review from lmg-anon May 20, 2024 08:44
@lmg-anon
Copy link
Owner

As I already said, I'd really appreciate it if you could handle the indexedDB stuff. Also I absolutely do not mind if you delay merging this PR until that is addressed.

No problem. I will try to handle it and commit to your branch before merging the PR.

@neCo2
Copy link
Contributor Author

neCo2 commented May 20, 2024

Thanks a lot. Let me know if there's anything else you need me to do.

@lmg-anon
Copy link
Owner

lmg-anon commented May 20, 2024

Alright, I think this is it. If possible, I would like you to take a second look to see if you find anything that might have broken after these changes, but otherwise this should be fine to merge now.

@neCo2
Copy link
Contributor Author

neCo2 commented May 20, 2024

Imports weren't working as templateStorage wasn't an argument for App() and the function still used sessionStorage.saveTemplates, but that's the only issue I found.

@lmg-anon
Copy link
Owner

lmg-anon commented May 20, 2024

Nice, thanks for another great contribution!

@lmg-anon lmg-anon merged commit 15acb02 into lmg-anon:main May 20, 2024
@neCo2
Copy link
Contributor Author

neCo2 commented May 21, 2024

Thank you for polishing it up!

@neCo2 neCo2 deleted the feature/instruct-templates branch May 21, 2024 08:33
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.

2 participants