Remove intent's syntetic ID construct and use name #10013
Conversation
Fixes #8974 Intent IDs were basically hashed from intent's labels (aka names?) and were in number format. If we change this to a string to make sure scientific notation does not lose precision, we essentially can use the label in place. Intent labels are unique and much more human readable. Does the job in a simpler way.
tayfun
requested review from
a team and
tttthomasssss
and removed request for
a team
|
I don't think a changelog is needed here because it is not a user facing change. |
There was a problem hiding this comment.
I think it makes sense having a changelog entry as the data that is sent to the action server for custom actions will now be different. And probably worth explaining that they can get the equivalent value by hashing the name too.
Also can you check we don't still have the id in any test events in unit tests? e.g. tests/core/test_actions.py
* Create stubs to start filling out Aciel's snippet Co-authored-by: Aciel Eshky <a.eshky@rasa.com> * Sketch out basics of CLI arguments * Start changing over args to new format * Make CLI match description from design pass * Add tests * Switch over timestamps to event IDs * Fix tests now event IDs are supported * Start work on doc comments * Switch over event_id fix Also removes otherwise-unused import to stop circularity * Address PR comments * Fix incorrect file extension * Codeclimate * Update rasa/core/evaluation/marker_base.py Co-authored-by: Kathrin Bujna <kathrin.bujna@gmail.com> * Address PR comments * Move test to marker * Rename dialogue to session Co-authored-by: Aciel Eshky <a.eshky@rasa.com> Co-authored-by: Kathrin Bujna <kathrin.bujna@gmail.com>
Thanks, good catch about other tests with ID in it :) Somehow it is not easy to grep them all. I have fixed all I've found and added the changelog now. |
There was a problem hiding this comment.
LGTM in general 🤖!
One small thing, wasn't the intent id exposed to users via the Events API in the rasa-sdk (i.e. inside UserUttered)? And if so, this could potentially cause unexpected crashes for any user who - for one reason or another - relies on the intent id for some processing, right?
@tttthomasssss , yes you're essentially right. This will be a backwards incompatible change for version 3. Rasa X may have to change some stuff including this change and as you have said, other users which rely on Intent ID too. Recommendation is to use intent name and if they can't, they can simply hash the name to get same number as before. |
Fixes #8974
Intent IDs were basically hashed from intent's labels (aka names?) and
were in number format. If we change this to a string to make sure
scientific notation does not lose precision, we essentially can use the
label in place. Intent labels are unique and much more human readable.
Does the job in a simpler way.
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)