Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create & manage LanguageModels and APIService (+add Llama 3) #389
Create & manage LanguageModels and APIService (+add Llama 3) #389
Changes from 20 commits
d2f16fc
c6ce6ea
43a2194
5544f1c
1a751a8
1d5eb1b
d7ad4f7
34ceb46
e4ed8f2
c6ea9a6
d6cb747
f3a9503
46b6806
c9bab8d
8b51f04
b36f264
fa52677
640a8f2
9651eb6
685dda6
2e4f6ac
8594d60
7abc447
752e115
407f757
23b901d
d1fab25
228dcb4
7e1512c
a13efbb
ad1cd40
f6e9fb7
90f8491
8061ccf
b8a3652
ccd8176
8c902d9
f0190b0
8b9a7d9
2d37035
236eaa6
3c1b59b
5c3ae61
c215eec
c6dad30
d6105c0
3594426
5e55b5d
4fd47bd
c371ff9
3ea3c9a
55f971e
1c0e70f
b7ccb75
75ab8ff
fb2bb9f
4e19856
992160e
4d625c1
885f17a
c5e7d73
d5a8882
86c91f2
a72c947
0b673c2
4859770
361cb25
c24c8d4
042261e
685614d
09dee26
a8a84da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there any way I could alter the URL and succeed at viewing another user's model? Let's prevent that, just to be safe:
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.
There was a test "should not update other user's language_model" for the controller, I added now also, "should not allow viewing other user's records" So I think no code change is needed.
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.
Is there any way that a user can trigger deletion of a language model? Is this preventing someone faking a form submission? If that's why it's here, let's move it to the controller. That feels like the better place for this check to be.
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 is the right place. There is a delete button for language models
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.
What happens if you remove all of this
destroy_in_process
logic in all of the models? What's the symptom you're experiencing that this solves for? Because I thought we solved this in the other PR in a way that made us no longer need this extra logic.My brief recollection was:
destroy
we simply callmodel.deleted!
and that sets the timestamp. So, for example, a controller destroy action would be updated to usedeleted!
dependent: :destroy
should work just fine without the need for this extra logicBut I realize there may be some consideration that I'm missing.
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.
Ok I'll save this for another session, tomorrow ?
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.
Ok made change, there's probably a gem for this. Look one up?
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.
Applied, see last commit, f0190b0
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 branching doesn't feel quite right. Why are there any assistants that do not have an
api_service
? Why don't we just create api_services for all of our system assistants? If we keep this inconsistency then we'll have to add these sprinkle these conditional checks throughout the code, whereas if we resolve the inconsistency then we can always assume an assistant has an api_serviceThere 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.
Not sure, it seems to me then we'll have the same thing with the OpenAI/Anthropic URL; we don't want to configure those URLs in our database do we?
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 we should configure those URLs in our database. The fact that we have two different ways of getting a URL is a good clue. If some assistant’s don’t have an api_service and others do, we’ll surely end up adding more conditionals over time to account for that difference.
If someone wanted to switch from OpenAI’s servers to using Microsoft Azure for all their GPT-4 needs, this would be the place to change it. It would be really nice to be able to edit that within the app. Also, I think we’ll inevitably end up adding additional logic to the API Service abstraction. Now that it exists, it’s a good place to house certain things. For example, one thing I’m working on is voice mode for the app. I did some digging and discovered that ChatGPT layers on some additional custom instructions when you activate the microphone. The GPT is told “the text you generate will be spoken so don’t use bullet points”.
This is not something I want to do per assistant or even per language model, I think it’s a setting which I would add to the api_service model. I’ll pre-populate some “voice instructions” will get concatenated to your Assistant’s custom instructions, but if you want to tweak that setting it would get tweaked at this low level and apply to all assistants.
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.
If you're thinking about changing these URLs then they would need to be user-level. So every user will have these user-level api-services, implying the existing language_models records we created with the db-migration will be moved(copied) to user-level too then? It does sound consistent, but I wanted to make sure I understand your suggestion right.
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.
Yes, that's right. So the Assistant > LanguageModel > APIService stack is all per-user. (Maybe some future "teams" features could cause us to revisit that with a more sophisticated permissions model...)