Conversation
giorgi-o
left a comment
There was a problem hiding this comment.
LGTM, this is great, thank you!
To answer your questions:
PpaRelevantEventSelector.filter_data is simplified from the Rust API. The Python bindings take int | None, where int matches events with that exact filter_data value, and None matches all events. A Python callable would require acquiring the GIL for every event checked, and I wasn't sure if that kind of latency is OK. Do we want to support the Rust API closure in a future PR, or is the simplified version sufficient for these bindings?
AFAIK grabbing the GIL is just a mutex lock, which is super duper fast in the uncontested case. So the case where this might cause a problem is if:
- the python code calls pdslib
- pdslib releases the GIL
- another python thread grabs the GIL and doesn't let go
- pdslib can't grab the GIL again
But I don't think we will encounter that scenario in our Python tests. Plus in this case, it would also cause a latency spike when the Rust code re-acquires the GIL when it eventually returns from the function back into python code.
So this version is fine for now, and if we want to extend it we can :)
Experimental features from the Rust library (unfiltered_bin_values, oob_filters) are not currently exposed in these bindings. Do we want to add these in a future PR?
Maybe, but really depends what we end up using these bindings for (other than AttributionML).
This PR exposes both PpaHistogramConfig and DirectPpaHistogramConfig for parity with the Rust API. Do we want to keep both of these in the bindings, or did we want to simplify?
To be honest I forgot that the Direct version existed! Now that they're implemented we can keep both, but I don't think the Direct variant has much use anymore (not sure what it's doing there in the first place tbh).
Thanks for the PR!
|
Out of curiosity, does this code even release the GIL? Don't you have to call |
@giorgi-o Oh yeah you're right, we don't release the GIL anywhere, so that wouldn't be the concern with using python callables in our case. The overhead would be from going into python for every event instead of staying in rust land I suppose? |
Makes sense. We can keep these in the icebox for now, and add them if we need them!
I'd be happy to remove direct if you think it's just noise in the bindings, up to you :) Thanks for the review!! |
|
No reason to remove Direct now, but if it causes any maintenance burden then off to the chopping block it goes. |
Add Python bindings for
compute_report(). (closes #100). Uses PyO3.Dependencies:
UriSet, used byEventUris,ReportRequestUrisEventUris, used byPpaEventPpaEvent, used byPds.register_event()(in order to register events to report on)ReportRequestUris, used byPpaHistogramRequestPpaHistogramConfig, used byPpaHistogramRequest.new()DirectPpaHistogramConfig, used byPpaHistogramRequest.new_direct()RequestedBuckets, used byPpaRelevantEventSelectorPpaHistogramRequest, used byPds.compute_report()Pds, implementsregister_event()andcompute_report()Note: These all link to line numbers. GitHub seems to be having an issue where they're stripped if you just click on the link. However, if you cmd+click or shift+click to open in a new tab, they work correctly.
TODO:
UriSet(a6a17f7)EventUris(5e6cca3)ReportRequestUris(080ca04)PpaEvent(b32cc7f)PpaHistogramConfig(f56e645)DirectPpaHistogramConfig(f56e645)RequestedBuckets(52bcd44)PpaRelevantEventSelector(8534d72)PpaHistogramRequest(72413a0)Pds(register_event, compute_report) (2457d7d)Questions
PpaRelevantEventSelector.filter_datais simplified from the Rust API. The Python bindings takeint | None, whereintmatches events with that exactfilter_datavalue, andNonematches all events. A Python callable would require acquiring the GIL for every event checked, and I wasn't sure if that kind of latency is OK. Do we want to support the Rust API closure in a future PR, or is the simplified version sufficient for these bindings?unfiltered_bin_values,oob_filters) are not currently exposed inthese bindings. Do we want to add these in a future PR?
PpaHistogramConfigandDirectPpaHistogramConfigfor parity with the Rust API. Do we want to keep both of these in the bindings, or did we want to simplify?