-
Notifications
You must be signed in to change notification settings - Fork 16
Add new BID_VALUATION intent and additional payload for intermediate transformation #16
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
Open
arpadmiklos-ttd
wants to merge
3
commits into
main
Choose a base branch
from
bid-valuation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
With this we can one can take the deals or segments from the RTB section and interpret the host wants those scored.
For discussion:
What should the semantic be if the caller/host passes imp[].pmp.deals[] or user.data[].segment[] as empty lists? Invalid request, or would the expectation be for it to still return something? Because for example, if the container receives an embedding it could issue ACTIVATE_DEAL and ACTIVATE_SEGMENT and at the same time a ADJUST_BID_VALUATION for those. While applicable_intents could be a signal of the type of types of IDs expected back in this scenario, I wonder if intents should support inputs other than what one may shoehorn as an extension, for example, in this empty list scenario where the container is supposed to give it its best shot, a topK input could be useful. And yes, this could also just be default configuration of the container or come through an extension.
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.
These IDs are representations of an advertiser's entities on the buyer side, and the purpose of this
BID_VALUATIONaddition is to allow third parties to contribute to the advertiser asset (say, campaign or ad group) valuation decision made by the DSP against the incoming bid request. As such, these do not correlate to individual deals or contextual segments or any other field in the bid request.IMO if we want to add functionality to attribute relative value/worth to deals or segments, that should be covered by another intent separate from this PR, and the discussion point you raised would be germane to that.
On a closely related note, I think the standard could be improved in terms of either explicitly associating intents and payloads more unequivocally, or at least clarifying expectations. For example, a container responding with mutations using the
ACTIVATE_SEGMENTSintent could well supplyIDsPayloadas sets of flat segment IDs, orDataPayloadinstances that have segments plus and additional name and ID or even content IDs, and I'm not sure what the expectation from an orchestrator is in this case.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 got ahead of myself by thinking of these object IDs as references to objects in a partial OpenRTB payload. From there, I moved toward semantics that could be implemented today using deal IDs and segment IDs as examples. More generally, though, I was exploring whether this could be generalized regardless of ID type—particularly the semantics of an empty list, where the orchestrator could treat the container as a recommender. But you may be right that recommendation or candidate discovery belongs in a separate intent specification altogether.
Setting that discussion aside, do you envision distinct ID-type-specific valuation containers, where each container can score only one type of ID? If so, would there be an implicit, out-of-band agreement about what the opaque IDs represent for that intent?
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.
The current 1.0 version of this standard doesn't specify such ID types even though
IDsPayloadis already used in different contexts, that's why I didn't add ID types here. When someone works with us to implement a container that supportsADJUST_BID_VALUATION, they will know what to expect based on out-of-band conventions (not part of this spec).Indeed we probably should add an ID type field, or create different variants of the
IDsPayloadmessage to ensure these payload IDs align with the expected type and that the container can actually work with them. Unfortunately, both would be a breaking change.