-
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
feat(pricing): expand gateway max price to set per capability/model id #3116
feat(pricing): expand gateway max price to set per capability/model id #3116
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.
Great work @ad-astra-video! I tested -maxPricePerCapability
with AI model pricing and it works correctly.
I also regression tested -maxPricePerUnit
to ensure price controls still work with transcoding. Using the H264 capability as the default price index is a creative approach that should allow us to set prices per codec one day in the future.
I tested livepeer_cli
with both options 16, 15 and found the price changes also work correctly there. I made a few edits to setMaxPriceForCapability
to prevent a runtime error and updated the test TestMaxPricePerCapability
, see my suggestions below.
Overall, changes LGTM!
Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
@eliteprox @rickstaa I agree with all suggestions and committed the suggested changes. |
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.
Changes LGTM. Approved pending review from @rickstaa
cmd/livepeer/starter/starter.go
Outdated
if p.Gateway == "" { | ||
p.Gateway = "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.
What is this field used for if, from what I understood, this is a config on the gateway itself? I thought this would make sense on an orchestrator config to set prices for specific gateways instead.
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.
Removed Gateway field for now. There was a time that this logic was going to be re-used for the Orchestrator as well to set price per gateway/pipeline/model_id but I think we found a way to re-use the aiModels.json config parsing to set price per gateway at the Orchestrator. Can always add it back in if needed.
Confirmed test still passed.
cmd/livepeer/starter/starter.go
Outdated
prices := make([]ModelPrice, len(pricesSet.CapabilitiesPrices)) | ||
for i, p := range pricesSet.CapabilitiesPrices { | ||
if p.Gateway == "" { | ||
p.Gateway = "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.
nit: Could be nice to support this defaulting for the model_id
field as well. Then one can specify only the pipeline for it to be the default price for the pipeline
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 a good idea. I removed the p.Gateway check and changed it to setting p.ModelID to 'default' if not set specifically.
core/ai.go
Outdated
//no capability description matches name | ||
return Capability_Unused |
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 wonder if this should be an explicit error instead (or the caller check this Unused
value) to avoid silent bad configurations. Otherwise one might be missing a configuration that they swear they are making on the ir JSON config file which could have bad implications (e.g. paying more than they were willing to). In general I think it's better to fail explicitly for bad configs than to ignore them.
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 agree that it would be better to update this to return (Capability, error)
to make it more obvious it should be checked by the caller.
Updated and added a test for this function in ai_test.go
…, assign modelID to 'default' if not set, add/update tests
@victorges @rickstaa updates made for review comments. Small merge conflict on a log line that looked the exact same resolved as well. |
@ad-astra-video the code looks good 🚀. However I was unable to build this on my system 🤔. Could you check if you pushed the latest changes? GO111MODULE=on CGO_ENABLED=1 CC="" CGO_CFLAGS="" CGO_LDFLAGS=" " go build -o "./" -tags "mainnet" -ldflags="-X github.com/livepeer/go-livepeer/core.LivepeerVersion=0.7.6-cdd74620" cmd/livepeer/*.go
# github.com/livepeer/go-livepeer/server
server/ai_session.go:206:3: undefined: Models
server/ai_session.go:208:29: capabilityConstraints[cap].Models undefined (type *"github.com/livepeer/go-livepeer/core".PerCapabilityConstraints has no field or method Models)
server/ai_session.go:209:14: undefined: core.NewCapabilitiesWithConstraints
server/broadcast.go:102:50: netCaps.CapabilityConstraints undefined (type *"github.com/livepeer/go-livepeer/net".Capabilities has no field or method CapabilityConstraints)
make: *** [Makefile:100: livepeer] Error 1 |
@rickstaa updated and re-tested this. docker image build available at |
cmd/livepeer/livepeer.go
Outdated
@@ -135,6 +135,7 @@ func parseLivepeerConfig() starter.LivepeerConfig { | |||
cfg.OrchPerfStatsURL = flag.String("orchPerfStatsUrl", *cfg.OrchPerfStatsURL, "URL of Orchestrator Performance Stream Tester") | |||
cfg.Region = flag.String("region", *cfg.Region, "Region in which a broadcaster is deployed; used to select the region while using the orchestrator's performance stats") | |||
cfg.MaxPricePerUnit = flag.String("maxPricePerUnit", *cfg.MaxPricePerUnit, "The maximum transcoding price per 'pixelsPerUnit' a broadcaster is willing to accept. If not set explicitly, broadcaster is willing to accept ANY price. Can be specified in wei or a custom currency in the format <price><currency> (e.g. 0.50USD). When using a custom currency, a corresponding price feed must be configured with -priceFeedAddr") | |||
cfg.MaxPricePerCapability = flag.String("maxPricePerCapability", *cfg.MaxPricePerCapability, `json list of prices per capability/model or path to json config file. Use "model_id": "default" to price all models in a pipeline the same. Example: {"capabilities_prices": [{"pipeline": "text-to-image", "model_id": "stabilityai/sd-turbo", "priceperunit": 1000, "pixelsperunit": 1}, {"pipeline": "upscale", "model_id": "default", priceperunit": 1200, "pixelsperunit": 1}]}`) |
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.
Like the format! Do we want to add the currency
field in there as well as an example?
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.
Can we change this to snake case to keep the JSON convention. For this we can also change the PricePerGateway
in this format (see https://linear.app/livepeer-ai/issue/AI-574/change-priceperbroadcaster-json-input-keys-to-recommended-conventions).
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.
Updated. Will update pricePerGateway to same in separate PR.
monitor/census.go
Outdated
mMinGasPrice *stats.Float64Measure | ||
mMaxGasPrice *stats.Float64Measure | ||
mTranscodingPrice *stats.Float64Measure | ||
mTranscodingPricePerCapability *stats.Float64Measure |
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.
Can we name it PricePerCapability
?
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.
Updated
server/broadcast.go
Outdated
return price | ||
} | ||
|
||
func (cfg *BroadcastConfig) GetCapabilityMaxPrice(cap core.Capability, modelID string) *big.Rat { |
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 one can be lowercase since it is not used outside this file :).
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.
Updated
This commit removes the BroadcastConfig initialisation method out of the starter to keep all logic inside the server module.
server/broadcast.go
Outdated
|
||
return &BroadcastConfig{ | ||
maxPricePerCapability: map[core.Capability]map[string]*core.AutoConvertedPrice{ | ||
core.Capability_H264: 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.
As discussed in our chat this can be core.Capability_Unused.
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.
Updated
cmd/livepeer/starter/starter.go
Outdated
@@ -1810,6 +1837,58 @@ func getGatewayPrices(gatewayPrices string) []GatewayPrice { | |||
return prices | |||
} | |||
|
|||
type ModelPrice struct { | |||
Gateway string |
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 field is a left over of another feature 🤔? Couldn't see it used.
This commit improves the parsing of the maxPricePerCapability config so that it doesn't throw nil errors and defaults to the regular transcoding values. It also throws clear log statments when config values are incorrect.
This commit simplifies the PipelineToCapability because we don't use unicode in our capability naming.
This commit applies some minor code improvements.
This commit fixes small typo in the 'price_per_capability' metric.
This commit improves the defaults used in the setBroadcastMaxPricePerCapability CLI option.
This commit ensures that we unsubscribe to the priceFeed when a model maxPrice is changed.
This commit applies some common go code conventions to the NewAISessionSelector function.
This commit optimizes the code a bit.
….com:ad-astra-video/go-livepeer into ai-video-set-max-price-per-capability-rebase
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 thanks!
This PR allows setting prices per capability and model id.
Current Implementation
Gateways currently use one MaxPrice set at launch with
maxPricePerUnit
flag and can be adjusted with thesetBroadcastConfig
endpoint of the cli webserver while the gateway is running.Enhancement Requested
Gateways on the AI subnet need the ability to filter orchestrators on price for different pipelines/model ids. Transcoding would also benefit from this when different encoding codecs are enabled on the network. The single MaxPrice needs to be updated to enable setting prices per capability (e.g. text-to-image and image-to-video) separately as well as allow the ability to price models within the capabilities separately. The single MaxPrice will function as the overall default if no prices are set for specific pipelines and/or models. The separate pricing needs to also allow setting a price per capability for all models in a capability.
Proposed implementation Changes
A
maxPricePerCapability
launch flag is added to accept json configuration of maximum prices for a capability/model. The json configuration uses themaxPricePerCapability
flag configuration to provide consistency in how prices are set on the network. Theserver.BroadcastCfg
is updated on the Gateway to store prices per capability/model id that can then be queried to provide the appropriate MaxPrice to use when selecting orchestrators in the selection algorithm. Theserver.BroadcastCfg
is also extended to add new functions forGetCapabilitiesMaxPrice
andGetCapabilityMaxPrice
to provide ability to get correct price with provided capabilities of the request. TheGetCapabilitiesMaxPrice
returns a total MaxPrice for the capabilities included in the request. If no specific max prices is set for the capabilities requested, the MaxPrice is used.GetCapabilityMaxPrice
returns the max price set for that capability/model id or zero if not set.The base
MaxPrice
andSetMaxPrice
functions are updated to set pricing of H264 encoding capability to allow extending this to provide different prices for different codecs in the future. ModelID is not used for transcoding so expected to always be "default".