allow scientific price notation #3253
Conversation
rickstaa
force-pushed
the
allow_scientific_price_notation
branch
from
120a185 to
f270b11
Compare
This commit ensures that orchestrators can specify pricing using the scientific notation.
rickstaa
force-pushed
the
allow_scientific_price_notation
branch
from
f270b11 to
7448c45
Compare
| @@ -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. | |||
There was a problem hiding this comment.
@leszko, @ad-astra-video I noticed I forgot this in the last pull request.
This commit removes some unnesesary changes in the TestParsePricePerUnit function I introduced.
|
The docs also have been updated in livepeer/docs#689. |
|
|
||
| // 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]*)?$`) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, I'm fine even with some "slight" non-backward-compatibility as long as it's ok with Orchestrators.
| @@ -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 { | |||
There was a problem hiding this comment.
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?
|
|
||
| // 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]*)?$`) |
There was a problem hiding this comment.
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?
| }, | ||
| { | ||
| name: "Valid price with capital scientific notation and currency", | ||
| pricePerUnitStr: "1.23E2USD", |
There was a problem hiding this comment.
Yeah I think having a space after the exponential notation would be helpful
| if s.Currency == "" { | ||
| s.Currency = currency | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
What does this pull request do? Explain your changes. (required)
This pull request ensures that orchestrators can specify pricing using the scientific format.
Specific updates (required)
How did you test each of these updates (required)
Does this pull request close any open issues?
Checklist:
makeruns successfully./test.shpass