[DISCO-3979] Add client variant logic to AMP thompson sampling#1338
[DISCO-3979] Add client variant logic to AMP thompson sampling#1338
Conversation
|
|
||
| # MERINO_WEB__API__V1__CLIENT_VARIANT_ALLOW_LIST | ||
| # list of client variants the merino will be aware about | ||
| client_variant_allow_list = ["engagement_guided_suggestions"] |
There was a problem hiding this comment.
I was thinking since there's going to only be one right now, this would be okay.
I'm not sure if we would ever have provider specific client_variants, and if so we might want to adjust the structure of this to maybe a dict
There was a problem hiding this comment.
I think it's OK for now, we can always store it in another form in Merino if needed.
|
|
||
| # MERINO_WEB__API__V1__CLIENT_VARIANT_ALLOW_LIST | ||
| # list of client variants the merino will be aware about | ||
| client_variant_allow_list = ["engagement_guided_suggestions"] |
| user_agent=user_agent, | ||
| source=source, | ||
| is_soft_pii=is_soft_pii, | ||
| client_variants=client_variants_list, |
There was a problem hiding this comment.
this keeps getting bigger and bigger! 😅 Curious if a middleware implementation would work for this and if it would make at any better 🤔
There was a problem hiding this comment.
Hmm, not sure if middleware would be helpful in this case. But we could group the related properties into nested sub-models :)
ncloudioj
force-pushed
the
client-variant-check
branch
from
a279d69 to
78eb229
Compare
misaniwere
left a comment
misaniwere
left a comment
There was a problem hiding this comment.
Can we add tests to verify when should_check_client_variants is True and we have a variant in the allow list?
4 participants
References
JIRA: DISCO-3979
Description
PR Review Checklist
Put an
xin the boxes that apply[DISCO-####], and has the same title (if applicable)[load test: (abort|skip|warn)]keywords are applied to the last commit message (if applicable)┆Issue is synchronized with this Jira Task