-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add support for niche targeting #865
Conversation
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 think the INSTALLED_APPS
check should be in the constructor. We can store a niche_weights=None
in the constructor and only fill it out if there's any candidate flights with niche_targeting
. We want to avoid doing this unless there's flights using the targeting. I'm really worried about this ballooning our time to ad.
This adds support for setting a threshold of distance, so flights outside of that distance are not shown. This has a few limitations, but could be a good first step for testing out a niche targeted flight, and see what it's CTR looks like.
Only run queries when a flight actually has niche_targeting set.
662636b
to
66cf03a
Compare
@davidfischer I updated this to be a bit smarter, and only do any embedding queries if the flights have niche targeting, and there's an embedding for the publisher URL. I think this should be safe enough to ship, since we can disable it by just removing the targeting on a test flight, and it will only do the queries when the test flight is in the candidate flights 👍 |
fc4d75f
to
e9234e8
Compare
# Apply niche targeting only when any flight has it. | ||
# This is to track whether we should do expensive distance queries. |
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 put this here so that we only run it when we hit an eligible flight that has niche targeting, so if eg. only house flights have it, it won't run if there's a paid ad.
get_niche_weights, | ||
) # noqa | ||
|
||
self.niche_weights = get_niche_weights( |
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.
Unless I'm reading this wrong, won't this get the distance for every advertiser? It'd be really nice if the embedding was just cached on the flight (or directly linked with a FK so select_related
just works). Then this check would just be a computation of getting the distance between the URL's embedding and the flight's embedding. Is that correct?
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.
Yea, we have a few options here -- but in general a flight doesn't have a single embedding, since it can have multiple ads that each have their own. We can in theory "average" them in some fashion, but that isn't what we're doing currently.
In the future we can definitely change how this works a bit more, but for now I'm mostly just trying to reduce overall queries when we aren't doing targeting, so we can start testing it in prod and see how slow it is.
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.
Alright. I think we'll have to calculate a flight embedding eventually but we can at least get a start with this.
Co-authored-by: David Fischer <david@readthedocs.org>
This adds support for setting a threshold of distance,
so flights outside of that distance are not shown.
This has a few limitations,
but could be a good first step for testing out a niche targeted flight,
and see what it's CTR looks like.