Convert Intent ID Hashes from Integer to String #8929
Conversation
|
While I think a string probably makes more sense, I'm a bit worried about the further implications of this change:
|
|
@joejuzl Do you think this solution still makes sense given the implications or if there's a better place in the code to cast it to avoid breaking changes? |
|
@b-quachtran could it be converted in the |
|
@joejuzl That would bring up object consistency issues between event brokers though right? I think this specific customer would be okay with a temporary fix using a custom Kafka Broker, but long-term does it make sense to consider changing type |
|
How about we use a config flag in |
b-quachtran
added
the
area:rasa-oss/event-brokers
| @@ -212,6 +212,9 @@ def _convert_intent_id_to_string(self, event: Dict[Text, Any]) -> Dict[Text, Any | |||
| event["parse_data"]["intent"]["id"] = str( | |||
| event["parse_data"]["intent"]["id"] | |||
| ) | |||
| for idx, parse_data in enumerate(event["parse_data"]["intent_ranking"]): | |||
| parse_data["id"] = str(parse_data["id"]) | |||
| event["parse_data"]["intent_ranking"][idx] = parse_data | |||
There was a problem hiding this comment.
I don't think you need this line (or the enumerate) as you are modifying the dict in the line above.
There was a problem hiding this comment.
@joejuzl intent_ranking is a separate field from intent and contains the full breakdown of intents + ID's, so the loop takes care of that piece. Does removing enumerate still make sense in this case?
There was a problem hiding this comment.
Sorry missed this before was merged.
I mean that parse_data is a direct reference to event["parse_data"]["intent_ranking"][idx] so when you update parse_data["id"] you are also also updating event["parse_data"]["intent_ranking"][idx]["id"] so no need to re-assign parse_data to event["parse_data"]["intent_ranking"][idx]. And thus you no longer need to know idx.
There was a problem hiding this comment.
@joejuzl Does python pass by reference though? I understood it as you have to use the index to update list elements in a loop.
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)