-
Notifications
You must be signed in to change notification settings - Fork 260
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
Priority vector re-prioritization proposal #329
Merged
+117
−16
Merged
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
c20ae4c
Update FLEDGE.md
MattMenke2 599f748
Update FLEDGE.md
MattMenke2 d4eeaa8
Update FLEDGE.md
MattMenke2 8b2b71e
Update FLEDGE.md
MattMenke2 893e837
Update FLEDGE.md
MattMenke2 156b3d0
Update FLEDGE.md
MattMenke2 8ef5cf4
Update FLEDGE.md
MattMenke2 31e29fd
Update FLEDGE.md
MattMenke2 8aaa6d5
Update FLEDGE.md
MattMenke2 75b398e
Update FLEDGE.md
MattMenke2 10560a1
Update FLEDGE.md
MattMenke2 06bcbd3
Update FLEDGE.md
MattMenke2 146fcdd
Update FLEDGE.md
MattMenke2 800597b
Update FLEDGE.md
MattMenke2 2d8b8bb
Update FLEDGE.md
MattMenke2 6369085
Update FLEDGE.md
MattMenke2 3f8b88d
Update FLEDGE.md
MattMenke2 b11d27d
Update FLEDGE.md
MattMenke2 ac326f1
Update FLEDGE.md
MattMenke2 1143633
Update FLEDGE.md
MattMenke2 0ae2dc7
Update FLEDGE.md
MattMenke2 de7578e
Update FLEDGE.md
MattMenke2 7d3f9df
Update FLEDGE.md
MattMenke2 692ab18
Apply suggestions from code review
MattMenke2 e109c0a
Update FLEDGE.md
MattMenke2 1a7e49a
Merge branch 'main' into priority-vector
MattMenke2 fb7e7a6
Update FLEDGE.md
MattMenke2 9c67b54
Apply suggestions from code review
MattMenke2 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
Update FLEDGE.md
commit d4eeaa8fd8bac4328ea8a223b82eca965cb4af4b
There are no files selected for viewing
This file contains 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.
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 way of sending data loses the association between keys and Interest Group (IG) names. For example, we might have IG 1 with key 'key1' and IG2 with keys ['key2', 'key3'].
This may become awkward and limiting for usage on the trusted server in the future. For example, it may often be the case that computing the bidding signals corresponding to some key may overlap with computing the perInterestGroupData for the corresponding IG.
Can we have an API that preserves the relationship between keys and IG names? For example, we could require keys and to be in the same order as IG names, with keys from different IGs separated by semicolons instead of commas.
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 does potentially make requests bigger, since we need to duplicate keys from multiple IGs. It would also require significant changes to the trusted server code. Are you thinking in terms of things that might be useful down the line, or that would be useful now, with the single perInterestGroupData field?
I'd hope that if something depends on key, it would be something that could be applied to all IGs with the same key, though that may be wishful thinking.
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.
At least with our usage, preserving the association means that we could make the request significantly smaller. If we cannot rely on the IG name->key association being available, the IG name potentially needs to be passed twice once as part of the key and once in the
interestGroups
field. There are ways one could imagine using the API where one would have many duplicate keys across IGs, but I am not sure if anyone uses the API in that way. (As far as I can tell those designs typically involve moving more computation onto the client, with corresponding latency and data integrity concerns.)Another related issue, supposing we wish to filter out the IG with negative priority with this API, we would also prefer to return empty bidding signal responses for the corresponding key(s). This either involves looking up a key to IG mapping on the server, duplicating data in the key, or doing redundant computations on the trusted server.
Whether or not all of this will be possible likely depends on details of the trusted server model, but I think it is desirable to keep the options open by preserving the information.
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.
Seems like for your use case, it would make more sense to make unique keys for each IG, rather than shared keys between IGs. One of the reasons to use keys, instead of just IG name, is so that IGs can share data.
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.
Yes, indeed that is how we are using the API. Please see my original concern about losing the association:
If we lose the association then this effectively limits the future ability of the trusted bidding server to use the interestGroupNames in computing the key response, or sharing computation between the two different outputs.