-
Notifications
You must be signed in to change notification settings - Fork 171
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
ai: add models webhook url param #3209
base: master
Are you sure you want to change the base?
Conversation
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.
Looking good, just a couple of small points 👍
@@ -116,7 +116,8 @@ type LivepeerNode struct { | |||
Database *common.DB | |||
|
|||
// AI worker public fields | |||
AIWorker AI | |||
AIWorker AI | |||
ModelsWebhook *ModelsWebhook |
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 we don't need this field for now as it's not used 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.
Isn't the point of this to fetch updated model configs? I added it to be used where needed doing updatedModels = ModelsWebhook.GetConfigs()
- but maybe @pwilczynskiclearcode knows better where this is needed to be used
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's assigned in here... and must live for the entire orchestrator lifetime and not be garbage collected or stopped.
I think it's 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.
Gotcha yep, it'll be integrated in a further PR
if err != nil { | ||
glog.Errorf("Error parsing -aiModels: %v", err) | ||
return | ||
} | ||
} | ||
|
||
for _, config := range configs { |
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.
@gioelecerati This entire loop needs to be run each time there are some changes to the ModelsWebhook.configs
@leszko Do you think this PR feature makes sense in context of remote worker, realtime AI video and future pricing policy plan |
@gioelecerati @thomshutt @mjh1 My thinking is that we still need this PR, but with the current setup is not the top priority. As per the latest discussion in Discord, we won't use neither External Container nor Remote Worker for Phase 1 of Realtime Video (Dec 12th deadline). So, we'll deploy a separate Orchestartor in each TensorDock machine. That means that restarting an Orchestartor is slightly less of a problem, because we'll have another Orchestrators deployed in different TensorDock machines. Still with Realtime Video, restarting Orchestrator is an issue, because it means a downtime for livestream, but that's a separate conversation. Long story short, I think that if it's not too much effort, then let's just take it to the finish line. However, if there are some other tasks, I think it's ok to park it for now and revisit in Dec/Jan. |
What does this pull request do? Explain your changes. (required)
Specific updates (required)
aiModelsWebhookUrl
param, accepting a string which can be either a webhook url or a file pathWebhook.GetConfigs()
returns the most recent configHow did you test each of these updates (required)
unit tests
Does this pull request close any open issues?
Checklist:
make
runs successfully./test.sh
pass