Introduce a product classifier #6680
Conversation
akatsoulas
force-pushed
the
product-classifier
branch
from
8c88f36 to
082d72b
Compare
There was a problem hiding this comment.
Overall, I like this approach. I'd suggest one refinement. I think we should only run the product re-classification when we know that the only reason the question was marked as spam was because of its irrelevance to its original product. In other words, it was a legitimate support request, but not relevant to its original product classification. The spam classification step can provide that extra information in its result without changing its current scope.
I created a PR based on your work here that shows what I'm thinking. I tested it using two examples that Emil provided and it worked great.
akatsoulas
force-pushed
the
product-classifier
branch
from
082d72b to
1d2ba71
Compare
akatsoulas
force-pushed
the
product-classifier
branch
3 times, most recently
from
7cd06b3 to
d7d493e
Compare
akatsoulas
force-pushed
the
product-classifier
branch
from
d7d493e to
678b098
Compare
kitsune/questions/utils.py
Outdated
| and hasattr(question, "product") | ||
| and getattr(question.product, "title", None) != new_product_title |
There was a problem hiding this comment.
Nit. The hasattr and getattr aren't necessary, and I think using:
and question.product
and (question.product.title != new_product_title)is more readable too.
kitsune/questions/utils.py
Outdated
| and hasattr(question, "product") | ||
| and getattr(question.product, "title", None) != new_product_title | ||
| ): | ||
| from kitsune.products.models import Product |
There was a problem hiding this comment.
This is not needed. Product is already imported above.
kitsune/questions/utils.py
Outdated
| try: | ||
| obj = (manager or model.objects).get(title=title) | ||
| except (model.DoesNotExist, getattr(model, "MultipleObjectsReturned", Exception)) as exc: | ||
| log.warning(f"{log_msg} Exception: {exc}", exc_info=True) |
There was a problem hiding this comment.
I think it would be nice to send this exception to Sentry, since it indicates that either the LLM has selected a bad product or topic title, or something's wrong with the products or topics that we've provided to the LLM in the prompt.
kitsune/questions/utils.py
Outdated
| if new_product := get_object_by_title( | ||
| Product, | ||
| new_product_title, | ||
| f"LLM suggested product {new_product_title} does not exist. Skipping product update.", | ||
| ): |
There was a problem hiding this comment.
I think this should also be restricted to active products, so I'd recommend adding manager=Product.active.
kitsune/questions/utils.py
Outdated
| if topic := update_fields.get("topic"): | ||
| question.clear_cached_tags() | ||
| question.tags.clear() | ||
| question.auto_tag() |
There was a problem hiding this comment.
It seems to me that we'd also need to re-do the question's tags for the case when the question's product has been updated (since the product slug is a tag), but the topic wasn't -- which I guess means the question was moved to a different product, but was also considered spam under that product? Maybe impossible, but maybe not? I'm thinking that it would be better to do something like the following instead:
# Always update the tags, because we've updated product and/or topic fields.
question.clear_cached_tags()
question.tags.clear()
question.auto_tag()
# Only flag the question if we've updated the topic.
if topic := update_fields.get("topic"):
flag_question(
...
)
akatsoulas
force-pushed
the
product-classifier
branch
from
678b098 to
603bda6
Compare
No description provided.