Skip to content
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

use_entities is broken when domain is split #10137

Closed
3 tasks
hsm207 opened this issue Nov 10, 2021 · 18 comments
Closed
3 tasks

use_entities is broken when domain is split #10137

hsm207 opened this issue Nov 10, 2021 · 18 comments
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework cse-issues effort:atom-squad/2 Label which is used by the Rasa Atom squad to do internal estimation of task sizes. type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors.

Comments

@hsm207
Copy link
Contributor

hsm207 commented Nov 10, 2021

Rasa Open Source version

2.8.12

Rasa SDK version

2.8.2

Rasa X version

No response

Python version

3.8

What operating system are you using?

Linux

What happened?

Create a domain.yml with the following content:

intents:
  - greet:
      use_entities: [country]
  - goodbye:
      use_entities: []
  - affirm
  - deny
  - mood_great
  - mood_unhappy
  - bot_challenge
 
entities:
  - city
  - country

Create another domain yaml with the following content:

intents:
  - inform:
      use_entities: [city]

Reading the merged domain files using Domain.load shows that the intent inform is still using the entities city and country.

The workaround is to repeat the list of entities in the first domain file in the second domain file i.e.:

intents:
  - inform:
      use_entities: [city]

entities:
  - city
  - country

Here is the code to reproduce this behavior.

Command / Request

No response

Relevant log output

No response

Definition of done

  • Fix bug in 2.8 and 3.0
  • Create integration test to ensure this works as expected
  • Fixed merged into 2.8 and 3.0
@hsm207 hsm207 added type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors. area:rasa-oss 🎡 Anything related to the open source Rasa framework labels Nov 10, 2021
@TyDunn
Copy link
Contributor

TyDunn commented Nov 12, 2021

The fix for this issue will need to be backported to the last minor of the previous major (i.e. Rasa Open Source 2.8)

@raoulvm
Copy link
Contributor

raoulvm commented Nov 25, 2021

I made a showcase on a possible solution here: raoulvm@17b4d81 in branch https://github.com/raoulvm/rasa/tree/2.8.14-fix-domain_from_directory
based on Rasa 2.8.14. It is no complete PR but a showcase which works with our (Deutsche Telekom) model.

@TyDunn
Copy link
Contributor

TyDunn commented Nov 25, 2021

@raoulvm Awesome. Thank you so much! This will help us make progress quickly on this issue

@TyDunn TyDunn added priority:high effort:atom-squad/2 Label which is used by the Rasa Atom squad to do internal estimation of task sizes. labels Nov 25, 2021
@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

Happy to help. We are waiting for the fix ...

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

@raoulvm am working on this right now. I've identified the root cause but need to ensure my fix doesn't break other things ;) I'll message you if I need some help.

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

@TyDunn I've raised a PR for 2.8.x and added a unit test. I don't think we need an integration test as the bug was isolated to a quirk in one of the shared.core.domain class methods only...

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

I have a bad feeling.
@carlad could you please test with the following two configs:

intents:
  - play:
      use_entities:
        - ball
        - chess
 - stow_away:
     use_entities: true
entities:
  - ball
  - chess
intents:
  - support_banning:
      use_entities:
         - automatic_rifles
         - anti_person_land_mines
  - certify:
      use_entities: true
entities:
  - automatic_rifles
  - anti_person_land_mines
  - tanks

and post the results here?
Thanks

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

Spoiler, that is expected, when using domain.as_yaml(clean_before_dump=True):

version: '2.0'
session_config:
  session_expiration_time: 60
  carry_over_slots_to_new_session: true
intents:
- support_banning:
    use_entities:
    - automatic_rifles
    - anti_person_land_mines
- certify
- play:
    use_entities:
    - ball
    - chess
- stow_away
entities:
- anti_person_land_mines
- automatic_rifles
- ball
- chess
- tanks

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

This is what I get when I use your rasa_moodbot repo with this PR's fix:

❯ make check-bug
python main.py --domain_folder ./domain/bug
intent: certify
entities used:
  anti_person_land_mines
  automatic_rifles
  ball
  chess
  tanks


intent: play
entities used:
  anti_person_land_mines
  automatic_rifles
  ball
  chess
  tanks


intent: stow_away
entities used:
  anti_person_land_mines
  automatic_rifles
  ball
  chess
  tanks


intent: support_banning
entities used:
  anti_person_land_mines
  automatic_rifles
  ball
  chess

So yeah. There's some additional logic needed. Let me look at your fix.

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

Aside from the moral question if playing with anti_person_land_mines is ok, did you expect that?

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

Are you able to raise a PR from your branch against 2.8.x

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

You could just copy the code changes. I provided that as an example.

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

There are open topics though: If the domains are not loaded from a directory, but individually listed in CLI, the fix does not apply. The change probably would be similar, but I neither had time nor interest to tackle that.

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

You linked to the entire branch. Which files did you change?

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

Let me have quick look at the commits.

@raoulvm
Copy link
Contributor

raoulvm commented Nov 30, 2021

raoulvm@17b4d81
That commit contains the changes.

@carlad
Copy link
Contributor

carlad commented Nov 30, 2021

@raoulvm thanks! Let me add your changes to the PR...

@carlad
Copy link
Contributor

carlad commented Dec 28, 2021

@raoulvm this fix has now been released in 2.8.19: https://pypi.org/project/rasa/2.8.19/

@carlad carlad closed this as completed Dec 28, 2021
carlad added a commit that referenced this issue Dec 28, 2021
carlad added a commit that referenced this issue Jan 3, 2022
carlad added a commit that referenced this issue Jan 4, 2022
* add  #10137 changes to domain merging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework cse-issues effort:atom-squad/2 Label which is used by the Rasa Atom squad to do internal estimation of task sizes. type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants