-
Notifications
You must be signed in to change notification settings - Fork 189
Updates ai-revision workflow for upcoming manubot-ai-editor multi-provider feature #522
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
base: main
Are you sure you want to change the base?
Updates ai-revision workflow for upcoming manubot-ai-editor multi-provider feature #522
Conversation
agitter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These generalization changes make sense and will be fast to approve. Let me know when you are ready to take it out of draft.
.github/workflows/ai-revision.yaml
Outdated
| commit-message: 'revise using AI model\n\nUsing the ${{ inputs.model_provider }} model ${{ inputs.model }}' | ||
| title: 'AI-based revision using ${{ inputs.model }}' | ||
| author: OpenAI model ${{ inputs.model }} <support@openai.com> | ||
| author: ${{ inputs.model_provider }} model ${{ inputs.model }} <support@${{ inputs.model_provider }}.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that support email address always work for future providers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @agitter, thanks for the comment. Frankly, no, it won't always work; it's just chance that both OpenAI and Anthropic have support emails at a domain that can be easily constructed from the provider string. I also don't know how helpful these providers' support emails would be anyway.
I could see a few ways forward here:
- change it to an email address for the Manubot AI Editor maintainers (my preferred option, since we'd be in a position to help if someone did have a comment)
- add a workflow parameter that specifies the author email address (not my favorite option, since it's one more thing for the user to have to worry about)
- use
<${{ github.actor_id }}+${{ github.actor }}@users.noreply.github.com>(the default for the create-pull-request action) which would associate the commit with the user who ran the workflow
I'll talk to @miltondp, the owner of Manubot AI Editor, about the above options and revise the PR accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both for your work and comments. I'd go either with the first/preferred option or the third one. Or even not specify any email address if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first option seems less suitable to me because the Manubot AI Editor maintainers are the authors of the commit. Those maintainers shouldn't appear as contributing directly to many downstream manubot manuscripts. The third option is a reasonable default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input, and that makes sense to me.
I've implemented option 3 as of commit 353d845. The commit author email is set to the same thing as the default value for the author field from the create-pull-request action inputs.
The display name remains <model_provider> model <model_name>; a example full author string would be:
openai model gpt-4-turbo <[email protected]>.
I've verified that the GitHub user who submitted the workflow is listed as a co-author on the commit; you can see an example of that in my fork of rootstock here: falquaddoomi@b8f36b0.
… patched manubot-ai-editor over it.
…ich uses the provider's default model.
… 'openai' with a warning
|
Summary of recent changes:
|
.github/workflows/ai-revision.yaml
Outdated
| required: true | ||
| type: string | ||
| default: 'ai-revision-gpt4turbo' | ||
| default: 'ai-revision-results' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not high priority, but is it possible to use a variable here so that if we revise multiple times using a different provider/model, the output branch will not override the (potentially) existing one?
Again, not high priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but that'd be a good idea; I'll look into it, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed in cc47cdd. The branch name field now defaults to ai-revision-{1}, where {1} is replaced with the final model name to match the older behavior. The provider could also be added to the branch name by specifying {0}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of 6b08f90, the new branch defaults to ai-revision-{0}-{1}, where {0} is the provider ("openai") and {1} is the model. I figured having more information in the branch name is better, especially since both values are short.
Added early-exit flags so that workflow stops if manubot-ai-editor fails
Adds provider to default branch name
Adds body to PR with some details about the revision and instructions.
…stalls latest manubot-ai-editor >= 0.5.0
miltondp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good, @falquaddoomi. I left a minor question/suggestion, if it makes sense. I would wait for @agitter for the final review and merge.
.github/workflows/ai-revision.yaml
Outdated
| env: | ||
| OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} | ||
| AI_EDITOR_LANGUAGE_MODEL: ${{ inputs.model }} | ||
| PROVIDER_API_KEY: ${{ secrets.PROVIDER_API_KEY || secrets.OPENAI_API_KEY }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also include Anthropic's, like in the suggestion below?
| PROVIDER_API_KEY: ${{ secrets.PROVIDER_API_KEY || secrets.OPENAI_API_KEY }} | |
| PROVIDER_API_KEY: ${{ secrets.PROVIDER_API_KEY || secrets.OPENAI_API_KEY || secrets.ANTHROPIC_API_KEY }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the comment. I'd originally added that to not break compatibility with users who hadn't set the new PROVIDER_API_KEY variable, but still had OPENAI_API_KEY set.
Now that I think about it, though, it's not necessary for it to check OPENAI_API_KEY, since OPENAI_API_KEY will take precedence over PROVIDER_API_KEY for OpenAI models, and if the user chooses an Anthropic model it'll fail earlier with a missing key rather than trying and failing to use an OpenAI PROVIDER_API_KEY with Anthropic later on in th eprocess.
I'll change it to just use secrets.PROVIDER_API_KEY; thanks again!
Co-authored-by: Milton Pividori <[email protected]>
We'll shortly be introducing the ability for
manubot-ai-editorto work with multiple providers (e.g., OpenAI, Anthropic, and more), facilitated by the LangChain library. These changes are included in PR manubot/manubot-ai-editor#71. That PR should be merged and a release created before we merge this PR, so this PR will remain a draft until that's completed.Since we'll potentially be implementing many providers, we decided to allow a generic environment variable,
PROVIDER_API_KEY, to specify the API key used for any one provider. Whilemanubot-ai-editorcould theoretically be run with multiple providers (each of which would need their own valid API key, of course), this particular workflow always runs the revision process with a single specific provider, so I assumed that usingPROVIDER_API_KEYwould be sufficient here. To not break things during the transition, the value will also be retrieved fromsecrets.OPENAI_API_KEYifsecrets.PROVIDER_API_KEYis unavailable; this will likely be removed down the road.I've also added
model_providerto the inputs to specify the model provider (i.e., "openai" or "anthropic" for now). I'm unsure if we should provide the list of options as a dropdown and just update this whenever we add new providers, or keep it as a free-text string. I've included the list of providers we currently support for now, but I'm considering removing that in favor of either having a set list of options that we update, or directing the user to the manubot-ai-editor docs for an up-to-date list.Finally, on @miltondp's advice, the default model engine is now
gpt-4orather thangpt-4-turbo.