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.

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

PMT API surface updates #1359

Merged
merged 2 commits into from
Dec 17, 2024
Merged

PMT API surface updates #1359

merged 2 commits into from
Dec 17, 2024

Conversation

csharrison
Copy link
Contributor

This PR fixes #1356 by allowing for processing modeling signals in reportWin out of the critical bidding path. This is beneficial for latency in cases where generating the encrypted modeling signals vector may impact auction latency.

Additionally, the new API unlocks the possibility to use PAA with sensitive (i.e. previously readable only within generateBid) data outside of the critical path.

Finally, the API is simplified in that we don't overload the modelingSignals output of generateBid anymore. We also
slightly backtrack on having the payload format be a raw Uint8 array, since we aren't quite ready to commit to that and we are investigating alternatives.

Feedback welcome!

@JensenPaul
Copy link
Collaborator

I generally like this.
I was wondering if we need a way to send the PMT data from reportWin() without invoking reportAggregateWin(), but perhaps this is a microoptimization and not worth it.
I was wondering if we need to support passing Uint8Arrays between generateBid() and reportAggregateWin(), but perhaps this is a microoptimization and not worth it. I suspect in the future we might want to consider supporting CBOR rather than JSON data coming from the real-time bidding servers (for faster parsing and more binary support) which may increase the demand for passing around Uint8Arrays.

@csharrison
Copy link
Contributor Author

  1. Avoiding reportAggregateWin. My hope is that reportAggregateWin can be a low-cost abstraction that doesn't require another path for sending PMT data. For instance, imagine if did have a path which could send PMT data without reportAggregateWin i.e. through another output of generateBid. In this case, couldn't we just consider instead supporting that path via a one-liner reportAggregateWin function if we properly thread the right data type through? This keeps the API surface simpler.
  2. Passing Uint8Array to reportAggregateWin . This approach I am more supportive of, which is to give reportAggregateWin the inputs it needs to avoid latency issues with marshalling / unmarshalling between the data formats between generateBid and reportAggregateWin. In my ideal world, the aggregateWinSignals could just be a generic JS object that could be structuredClone'd to reportAggregateWin. Do you think that's feasible? It would naturally allow us to support Uint8Array or any other format that we need for reporting.

The reason I avoided specifying (2) in this PR was to keep things simple, as we can always implement (2) in a backwards compatible way if we need to in the future. However, I am supportive of making this more flexible initially if you are.

@michaelkleber
Copy link
Collaborator

I think reportAggregateWin() is an excellent addition to the API surface.

It feels like a blemish to me that we need to configure its properties from inside reportWin(), because (a) that is such a different job than everything else reportWin() is doing, and (b) the "either configure rAW or use modelingSignals" mechanism is pretty awkward. I keep trying to think of an alternative where the rAW config is set statically as part of the IG — would that work as long as we include them in the k-anon check?

I can't find any really satisfying variant that maintains backwards compatibility, so it seems likely that you have thought through all these options already and have landed on the most reasonable one.

@csharrison
Copy link
Contributor Author

@michaelkleber see this section of the explainer for a discussion of doing this configuration at IG join time. If the argument doesn't convince you, happy to continue the argument here.

I want to mention that with the reportAggregateWin functionality, it is not just PMT that could use this kind of configuration, but also the Private Aggregation API (callable within reportAggregateWin). When the configuration for a PAA invocation is "public", we allow Private Aggregation API to:

  • Configure how many contributions there are in a report
  • append a context ID to PAA reports
  • (future extension) provide a cleartext batching ID to reports (see here)

I don't include these details in the PR, but it is a natural extension.

@michaelkleber
Copy link
Collaborator

Ahh yes, thank you for reminding me of that Alternative configuration options section of the doc. Good summary of the options and their relative merits.

Yup, I like reportAggregateWin a lot, and I can get on board with the use of the legacy reportWin function evolving into "Place where it's safe to do operations whose effect will be event-level observable." It's a shift in mental model, and means the old name is kinda inapt, but it gets the job done. LGTM.

@JensenPaul JensenPaul merged commit 890d1be into WICG:main Dec 17, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Dec 17, 2024
SHA: 890d1be
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@WICG WICG deleted a comment from indi269 Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider an API off the critical path for private model training
3 participants