Skip to content
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.

Sign up for GitHub

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

allow scientific price notation #3253

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

allow scientific price notation #3253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
847a0ee
chore(ai): update CHANGELOG_PENDING
rickstaa Nov 14, 2024
9ca55a4
feat: allow scientific price notation
rickstaa Nov 15, 2024
7448c45
chore(ai): update CHANGELOG_PENDING
rickstaa Nov 15, 2024
5233994
refactor: remove unnecessary test changes
rickstaa Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG_PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Unreleased Changes

- [#3253](https://github.com/livepeer/go-livepeer/pull/3253) - Allow orchestrators to specify pricing using scientific notation.
- [#3248](https://github.com/livepeer/go-livepeer/pull/3248) - Provide AI orchestrators with a way to specify `pricePerUnit` as a float.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leszko, @ad-astra-video I noticed I forgot this in the last pull request.


## v0.X.X

### Breaking Changes 🚨🚨
Expand Down
23 changes: 3 additions & 20 deletions cmd/livepeer/starter/starter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"os/user"
"path"
"path/filepath"
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -859,7 +858,7 @@ func StartLivepeer(ctx context.Context, cfg LivepeerConfig) {
// Prevent orchestrators from unknowingly doing free work.
panic(fmt.Errorf("-pricePerUnit must be set"))
} else if cfg.PricePerUnit != nil {
pricePerUnit, currency, err := parsePricePerUnit(*cfg.PricePerUnit)
pricePerUnit, currency, err := common.ParsePricePerUnit(*cfg.PricePerUnit)
if err != nil {
panic(fmt.Errorf("-pricePerUnit must be a valid integer with an optional currency, provided %v", *cfg.PricePerUnit))
} else if pricePerUnit.Sign() < 0 {
Expand Down Expand Up @@ -998,7 +997,7 @@ func StartLivepeer(ctx context.Context, cfg LivepeerConfig) {
// Can't divide by 0
panic(fmt.Errorf("-pixelsPerUnit must be > 0, provided %v", *cfg.PixelsPerUnit))
}
maxPricePerUnit, currency, err := parsePricePerUnit(*cfg.MaxPricePerUnit)
maxPricePerUnit, currency, err := common.ParsePricePerUnit(*cfg.MaxPricePerUnit)
if err != nil {
panic(fmt.Errorf("The maximum price per unit must be a valid integer with an optional currency, provided %v instead\n", *cfg.MaxPricePerUnit))
}
Expand Down Expand Up @@ -1290,7 +1289,7 @@ func StartLivepeer(ctx context.Context, cfg LivepeerConfig) {
pricePerUnitBase := new(big.Rat)
currencyBase := ""
if cfg.PricePerUnit != nil {
pricePerUnit, currency, err := parsePricePerUnit(*cfg.PricePerUnit)
pricePerUnit, currency, err := common.ParsePricePerUnit(*cfg.PricePerUnit)
if err != nil || pricePerUnit.Sign() < 0 {
panic(fmt.Errorf("-pricePerUnit must be a valid positive integer with an optional currency, provided %v", *cfg.PricePerUnit))
}
Expand Down Expand Up @@ -1985,22 +1984,6 @@ func parseEthKeystorePath(ethKeystorePath string) (keystorePath, error) {
return keystore, nil
}

func parsePricePerUnit(pricePerUnitStr string) (*big.Rat, string, error) {
pricePerUnitRex := regexp.MustCompile(`^(\d+(\.\d+)?)([A-z][A-z0-9]*)?$`)
match := pricePerUnitRex.FindStringSubmatch(pricePerUnitStr)
if match == nil {
return nil, "", fmt.Errorf("price must be in the format of <price><currency>, provided %v", pricePerUnitStr)
}
price, currency := match[1], match[3]

pricePerUnit, ok := new(big.Rat).SetString(price)
if !ok {
return nil, "", fmt.Errorf("price must be a valid number, provided %v", match[1])
}

return pricePerUnit, currency, nil
}

func refreshOrchPerfScoreLoop(ctx context.Context, region string, orchPerfScoreURL string, score *common.PerfScore) {
for {
refreshOrchPerfScore(region, orchPerfScoreURL, score)
Expand Down
88 changes: 0 additions & 88 deletions cmd/livepeer/starter/starter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,91 +330,3 @@ func TestUpdatePerfScore(t *testing.T) {
}
require.Equal(t, expScores, scores.Scores)
}

func TestParsePricePerUnit(t *testing.T) {
tests := []struct {
name string
pricePerUnitStr string
expectedPrice *big.Rat
expectedCurrency string
expectError bool
}{
{
name: "Valid input with integer price",
pricePerUnitStr: "100USD",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid input with fractional price",
pricePerUnitStr: "0.13USD",
expectedPrice: big.NewRat(13, 100),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid input with decimal price",
pricePerUnitStr: "99.99EUR",
expectedPrice: big.NewRat(9999, 100),
expectedCurrency: "EUR",
expectError: false,
},
{
name: "Lower case currency",
pricePerUnitStr: "99.99eur",
expectedPrice: big.NewRat(9999, 100),
expectedCurrency: "eur",
expectError: false,
},
{
name: "Currency with numbers",
pricePerUnitStr: "420DOG3",
expectedPrice: big.NewRat(420, 1),
expectedCurrency: "DOG3",
expectError: false,
},
{
name: "No specified currency, empty currency",
pricePerUnitStr: "100",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "",
expectError: false,
},
{
name: "Explicit wei currency",
pricePerUnitStr: "100wei",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "wei",
expectError: false,
},
{
name: "Invalid number",
pricePerUnitStr: "abcUSD",
expectedPrice: nil,
expectedCurrency: "",
expectError: true,
},
{
name: "Negative price",
pricePerUnitStr: "-100USD",
expectedPrice: nil,
expectedCurrency: "",
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
price, currency, err := parsePricePerUnit(tt.pricePerUnitStr)

if tt.expectError {
assert.Error(t, err)
} else {
require.NoError(t, err)
assert.True(t, tt.expectedPrice.Cmp(price) == 0)
assert.Equal(t, tt.expectedCurrency, currency)
}
})
}
}
17 changes: 17 additions & 0 deletions common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,3 +497,20 @@ func MimeTypeToExtension(mimeType string) (string, error) {
}
return "", ErrNoExtensionsForType
}

// ParsePricePerUnit parses a price string in the format <price><exponent><currency> and returns the price as a big.Rat and the currency.
func ParsePricePerUnit(pricePerUnitStr string) (*big.Rat, string, error) {
pricePerUnitRex := regexp.MustCompile(`^(\d+(\.\d+)?([eE][+-]?\d+)?)([A-Za-z][A-Za-z0-9]*)?$`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change backward-compatible? I see you use a different regex here, so I wonder, won't it break anything for the existing config?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick glimpse, it seems like the incompatibility would be if we had a currency being specified as [eE][0-9]* before, which will be detected as a scientific notation now. I think that case is pretty unlikely tho, but one solution could be to add a required space after the number in scientific notation (after \d+), which could be a good practice anyway for clearer 123e-12 USD instead of a single 123e-12USD. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine even with some "slight" non-backward-compatibility as long as it's ok with Orchestrators.

match := pricePerUnitRex.FindStringSubmatch(pricePerUnitStr)
if match == nil {
return nil, "", fmt.Errorf("price must be in the format of <price><exponent><currency>, provided %v", pricePerUnitStr)
}
price, currency := match[1], match[4]

pricePerUnit, ok := new(big.Rat).SetString(price)
if !ok {
return nil, "", fmt.Errorf("price must be a valid number, provided %v", match[1])
}

return pricePerUnit, currency, nil
}
117 changes: 117 additions & 0 deletions common/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/livepeer/go-livepeer/net"
"github.com/livepeer/lpms/ffmpeg"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestFFmpegProfiletoNetProfile(t *testing.T) {
Expand Down Expand Up @@ -476,3 +477,119 @@ func TestMimeTypeToExtension(t *testing.T) {
_, err := MimeTypeToExtension(invalidContentType)
assert.Equal(ErrNoExtensionsForType, err)
}

func TestParsePricePerUnit(t *testing.T) {
tests := []struct {
name string
pricePerUnitStr string
expectedPrice *big.Rat
expectedCurrency string
expectError bool
}{
{
name: "Valid input with integer price",
pricePerUnitStr: "100USD",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid input with fractional price",
pricePerUnitStr: "0.13USD",
expectedPrice: big.NewRat(13, 100),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid input with decimal price",
pricePerUnitStr: "99.99EUR",
expectedPrice: big.NewRat(9999, 100),
expectedCurrency: "EUR",
expectError: false,
},
{
name: "Lower case currency",
pricePerUnitStr: "99.99eur",
expectedPrice: big.NewRat(9999, 100),
expectedCurrency: "eur",
expectError: false,
},
{
name: "Currency with numbers",
pricePerUnitStr: "420DOG3",
expectedPrice: big.NewRat(420, 1),
expectedCurrency: "DOG3",
expectError: false,
},
{
name: "No specified currency, empty currency",
pricePerUnitStr: "100",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "",
expectError: false,
},
{
name: "Explicit wei currency",
pricePerUnitStr: "100wei",
expectedPrice: big.NewRat(100, 1),
expectedCurrency: "wei",
expectError: false,
},
{
name: "Valid price with scientific notation and currency",
pricePerUnitStr: "1.23e2USD",
expectedPrice: big.NewRat(123, 1),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid price with capital scientific notation and currency",
pricePerUnitStr: "1.23E2USD",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think having a space after the exponential notation would be helpful

expectedPrice: big.NewRat(123, 1),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Valid price with negative scientific notation and currency",
pricePerUnitStr: "1.23e-2USD",
expectedPrice: big.NewRat(123, 10000),
expectedCurrency: "USD",
expectError: false,
},
{
name: "Invalid number",
pricePerUnitStr: "abcUSD",
expectedPrice: nil,
expectedCurrency: "",
expectError: true,
},
{
name: "Negative price",
pricePerUnitStr: "-100USD",
expectedPrice: nil,
expectedCurrency: "",
expectError: true,
},
{
name: "Only exponent part without base (e-2)",
pricePerUnitStr: "e-2USD",
expectedPrice: nil,
expectedCurrency: "",
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
price, currency, err := ParsePricePerUnit(tt.pricePerUnitStr)

if tt.expectError {
assert.Error(t, err)
} else {
require.NoError(t, err)
assert.True(t, tt.expectedPrice.Cmp(price) == 0)
assert.Equal(t, tt.expectedCurrency, currency)
}
})
}
}
44 changes: 44 additions & 0 deletions core/ai.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/golang/glog"
"github.com/livepeer/ai-worker/worker"
"github.com/livepeer/go-livepeer/common"
)

var errPipelineNotAvailable = errors.New("pipeline not available")
Expand Down Expand Up @@ -82,9 +83,51 @@ type AIModelConfig struct {
Currency string `json:"currency,omitempty"`
}

// UnmarshalJSON allows `PricePerUnit` to be specified as a string.
func (s *AIModelConfig) UnmarshalJSON(data []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you cover different scenarios here, like price_per_unit can be a string or an int (or float?). Could you add some units for that to make sure it works for all these cases?

type Alias AIModelConfig
aux := &struct {
PricePerUnit interface{} `json:"price_per_unit"`
*Alias
}{
Alias: (*Alias)(s),
}

if err := json.Unmarshal(data, &aux); err != nil {
return err
}

// Handle PricePerUnit
var price JSONRat
switch v := aux.PricePerUnit.(type) {
case string:
pricePerUnit, currency, err := common.ParsePricePerUnit(v)
if err != nil {
return fmt.Errorf("error parsing price_per_unit: %v", err)
}
price = JSONRat{pricePerUnit}
if s.Currency == "" {
s.Currency = currency
}
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see what you are doing there. Is it worth having this logic compared to just keeping the currency in a separate field in the JSON? This provides multiple ways of doing the same thing and leads to ambiguous behavior (e.g. what happens if I specify both the "currency" and add a currency to the pricePerUnit?)

AFAIK the go JSON lib (and the JSONRat impl) already supports scientific notation, so nothing we need to do add that support there.

If we do go with the new way of specifying the price, I believe we should implement it for the other price config JSON's as well, like the video gateway for price per O.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking further about this, maybe we remove this change on the JSON config, which is redundant, and make it only for the CLI flags, wdyt?

default:
pricePerUnitData, err := json.Marshal(aux.PricePerUnit)
if err != nil {
return fmt.Errorf("error marshaling price_per_unit: %v", err)
}
if err := price.UnmarshalJSON(pricePerUnitData); err != nil {
return fmt.Errorf("error unmarshaling price_per_unit: %v", err)
}
}
s.PricePerUnit = price

return nil
}

// ParseAIModelConfigs parses AI model configs from a file or a comma-separated list.
func ParseAIModelConfigs(config string) ([]AIModelConfig, error) {
var configs []AIModelConfig

// Handle config files.
info, err := os.Stat(config)
if err == nil && !info.IsDir() {
data, err := os.ReadFile(config)
Expand All @@ -99,6 +142,7 @@ func ParseAIModelConfigs(config string) ([]AIModelConfig, error) {
return configs, nil
}

// Handle comma-separated list of model configs.
models := strings.Split(config, ",")
for _, m := range models {
parts := strings.Split(m, ":")
Expand Down
Loading