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

[Advanced Paste] Add Semantic Kernel opt-in to allow chaining of paste actions #35902

Open
wants to merge 68 commits into
base: main
Choose a base branch
from

Conversation

drawbyperpetual
Copy link
Collaborator

@drawbyperpetual drawbyperpetual commented Nov 12, 2024

Summary of the Pull Request

See linked issue for more information.

PR Checklist

Detailed Description of the Pull Request / Additional comments

There's quite a lot of refactoring and code-quality improvements here. A non-exhaustive list:

  • All transformations now work on and produce a 'virtual' in-memory clipboard representation (DataPackage) rather than on the system clipboard. The system clipboard is only read once per operation and written to only just before pasting (if needed). There are multiple reasons for this:
    • Prevents Semantic Kernel intermediate transformations from polluting the clipboard with intermediate data.
    • Allows Semantic Kernel tool-calls (and in fact, other transforms too) to easily run on the thread-pool without causing "calling application is not the owner of the data on the clipboard" issues.
    • Allows transformations to be unit-tested without manipulating the system clipboard.
    • Cleaner and faster as the actual clipboard is touched only once.
  • More Async within AdvancedPaste (this was also done as part of Additional Actions but there's much more of it now).
  • Paste actions now have a CanPreview property (and Image to Text has been marked as having it to allow it to be previewed)
  • lastQuery.json functionality is removed
  • IFileSystem is used within Advanced Paste to improve testability.

Validation Steps Performed

  • Tested new functionality
  • Tested existing Advanced Paste functionality

@htcfreek
Copy link
Collaborator

Two cents on this:

  1. We could use an expander on the settings page. The two settings for preview and advanced AI depends on the enable AI setting.
  2. Wondering if we need a policy for this? 🤔 Thoughts?

@drawbyperpetual
Copy link
Collaborator Author

Two cents on this:

  1. We could use an expander on the settings page. The two settings for preview and advanced AI depends on the enable AI setting.
  2. Wondering if we need a policy for this? 🤔 Thoughts?

@htcfreek Regarding the expander for Paste with AI, I think it isn't necessary - I think we'd want to highlight the new features. It's also consistent without the expander - see Mouse Utilities for example. Having said that, you've brought up the important point that the "Custom Preview" toggle being present is tied to AI being enabled. But in this PR, I've made it also work for Image to Text. So we should untie it from AI! I'll do that..

Regarding the policy, is there any benefit that we would get by adding one given that we already one for online AI models?

@htcfreek
Copy link
Collaborator

Regarding the policy, is there any benefit that we would get by adding one given that we already one for online AI models?

Two maybe benefits I am not sure about:

  • Prevent using gpt-4o and force gpt3-turbo.
  • Prevent that users create complex queries that costs many credits. (Maybe we can make a max steps limit policy?)

But I am not sure if these are things that require a policy. 🤔

@htcfreek
Copy link
Collaborator

Having said that, you've brought up the important point that the "Custom Preview" toggle being present is tied to AI being enabled. But in this PR, I've made it also work for Image to Text. So we should untie it from AI! I'll do that..

Then we should move the toggle into tge behavior settings section.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Nov 13, 2024
@jaimecbernardo
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jaimecbernardo
Copy link
Collaborator

Regarding the policy, is there any benefit that we would get by adding one given that we already one for online AI models?

Two maybe benefits I am not sure about:

  • Prevent using gpt-4o and force gpt3-turbo.
  • Prevent that users create complex queries that costs many credits. (Maybe we can make a max steps limit policy?)

But I am not sure if these are things that require a policy. 🤔

In terms of GPO for the feature, I'm thinking the open AI already has controls to limit credit usage / model usage per token, in case they're using a work API key 🤔 A new GPO seems not necessary at this point 😄

@drawbyperpetual
Copy link
Collaborator Author

@htcfreek I've moved the Custom Preview toggle into the Behavior section as you suggested.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Code LGTM.
Testing looks pretty good too. Some errors could be more explicit, like when an image can't be converted through text, but I don't think that's blocking.

Still checking on some verifications, so I'm not approving yet, but will approve after verifications are done. Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in for .87 Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants