gitbutler
gitbutler copied to clipboard
gitbutlerapp
DRAFT: AI Provider Settings
Looking really solid 💪
Thanks for taking a look, I realise it turned into quite a large PR so I'd be happy to talk over any of the changes if you have any questions/thoughts about any choices
For things operating on git like gitGetConfig, could we integrate them into an existing service or create a new one? I think we want to stick with using dependancy injection to keep the code testable, even though we currently don't have front end tests.
Everything else looks great to me!
A musing I had was, the aiService is more or less the summarizer with the creation of the provider/client just brought inside the class.
I'm going to have a think about this, because we're currently not succeeding at making the provider easy to inject
A musing I had was, the aiService is more or less the summarizer with the creation of the provider/client just brought inside the class.
I'm going to have a think about this, because we're currently not succeeding at making the provider easy to inject
I was concerned about how to test it partially because I've not tried vitest before apart from seeing someone use it on stream, so I thought I'd give it a go, and I've been able to reasonably cover most of the logic
Just leaving a note that I'll review the new changes as soon as I get a chance!
I've just seen the new context changes, I'll go rebase in a bit and now the userService can be gotten from the userContext, I'll have a go passing it or the token into the commit and branch methods... which I think I'll rename to summarizeCommit and summarizeBranch because it would then read as aiService.summarizeCommit
I've done the rebase. I'll do the tweaks I said I'll do in a little bit, but then I've got some other things I need to work on this weekend
I just sat down to look at this again. Sorry for my absence, I was deep in the weeds trying to wrap my head around both streams and dependency injection. ✌️
No worries @mtsgrd!
I've gone and done that refactor I mentioned.
Part of me is wanting to extract the prompt construction and post processing into their own functions to better test them. What are your thoughts on that?
@Caleb-T-Owens hey, sorry, I pushed here even though I meant to push to my own branch. I didn't realize GitButler can push to other peoples' forks. :)
Anyway, feel free to force push if you want to take some suggestions but tweak it. I just wanted to get this pushed before I go have some beers tonight. 🥳
@mtsgrd
hey, sorry, I pushed here even though I meant to push to my own branch. I didn't realize GitButler can push to other peoples' forks. :)
Anyway, feel free to force push if you want to take some suggestions but tweak it. I just wanted to get this pushed before I go have some beers tonight. 🥳
No worries! GitHub has an option to permit maintainers to push to your fork which is pretty nifty. I can now pull them into my branch with the upstream commits card now too 💪!
Enjoy your evening!
I see you've changed https://github.com/gitbutlerapp/gitbutler/pull/3084/commits/1247e5253ad8d3d9cc130599686075f8a39e7e58#diff-c828d978a2adbaa41707e94fea99a318b081ed67c3c3fa2fb610416dbc4431bfR23-R24 from using null to undefined. I typed that using null because invoke seems to be returning null but it was more inconvenient due to the bind value of text boxes wanting to receive string | undefined so I'm happy to make the move over. I am going to add in a ?? undefined to ensure our types are consistent with our actual values.
Unrelated to your changes, I was having a think about https://github.com/gitbutlerapp/gitbutler/pull/3084/files#diff-17085abea331547efcd2493dbd64eb700ea1f3108919330b2a6748eb56ab9060R30-R35 where we're saying $: setContext(...).
To me this seems a little odd because if we're in the scenario where data.something changes and we set the state, none of the getContext calls will react to that as their components won't have re-rendered. Aside from that, I was also under the impression that the load() method is awaited before the +layout.svelte is rendered meaning that the items in the data object shouldn't be getting updated after the +layout.svelte is rendered. If that assumption is true, doesn't that mean that we don't need to react regardless?
Perhaps I'm missing some sveltey wizardry that is going on in the background that I don't know about, so I'm keen to hear about this
Just quickly before I head out.
- Yeah let's convert the
nulltoundefined, and pretendnullbarely even exists in this code base - The services located under
/[projectId]/get update in the correspondinglayout.tsfile, but changing projects re-renders everything with agetContextcall, so we're cool for now. - I would perhaps prefer if we only had global services rather than some that depend on
projectId, but let's refactor that later
*edit: just want to say massive thanks, this feature is looking really sharp. 🙏
I'm pretty stoked how well written this feature has become. Feel free to land the changes at any time 👍
this is exciting but does it add ollama support? I'd like either ollama or grok or something that I get more choice or privacy with preferrably.
Hi @Leopere, this certainly sets the groundwork for adding in supporting custom models over a user provided API endpoint, but doesn't yet provide that option.
In my dev environment I hacked in a basic AIClient that talked to ollama (which took about 5 minutes to do), but it made it clear that there is a good portion of work that needs to be done, before these weaker models can be anywhere near useful.
I summarised what I deemed to be prerequisite on discord: https://discord.com/channels/1060193121130000425/1204524097241878629/1219776902978600970. (invite to the server if needed: https://discord.gg/B7rv6qBejb)
Yeah it’s kind of a shame that we’re not quite close to parity in oss to accomplish onprem llms yet
@Caleb-T-Owens @mtsgrd if you do new updated and this involves UI — include me to reviewers.
