-
Notifications
You must be signed in to change notification settings - Fork 7
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
summaries: refactor to use openai api model for inference #59
Conversation
7c24f74
to
cc85d55
Compare
9db544d
to
28c7987
Compare
28c7987
to
9008f9c
Compare
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.
Left some comments, PTAL. It's not clear to me how the 2 ways of running the OpenAI-compat app are attained. Wasn't the executor going to hit an internal OpenAI-compat service locally bypassing auth?
ea480d4
to
256d58a
Compare
256d58a
to
4cddaf6
Compare
* inference jobso of customers with existing open ai keys will run on open ai servers
d948388
to
1bcab62
Compare
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.
Left some comments, PTAL!
skynet/auth/openai.py
Outdated
open_yaml(openai_credentials_file) | ||
file_watcher = FileWatcher(openai_credentials_file, lambda: open_yaml(openai_credentials_file)) | ||
|
||
file_watcher.start() |
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.
When are we going to stop this?
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.
when the process stops. Is there other time when we'd want it to stop?
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.
Not really, in general I prefer explicit vs implicit. No big deal though.
log.error(f'Error while polling for file changes: {e}') | ||
break | ||
|
||
def start(self): |
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 guess there should be a stop here...
n_ctx=4096, | ||
n_gpu_layers=llama_n_gpu_layers, | ||
n_batch=llama_n_batch, | ||
global llm |
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 this initialize necessary? Can't we just create it at imprt time?
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 we move it like that, we need to also better isolate the summaries:dispatcher module to make sure it's not importing it indirectly.
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.
Does the dispatcher import anything from the processor? It kinda shouldn't.
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.
it imports the jobs module, which in turn has reference to the processor
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.
Gotcha. No strong opinion then.
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.
LGTM, excellent work! Since I opened the PR you need to approve it yourself lol
Pl squash-merge. |
No description provided.