core icon indicating copy to clipboard operation
core copied to clipboard

Alexa Intent: Use the 'id' field and expose nearest resolutions as variables

Open AzonInc opened this issue 2 years ago • 23 comments

Expose nearest Resolution (ID and Value) as Variables

Breaking change

Proposed change

Using the id value can be valuable if you don't want to use the id for entities but to process it in scripts for example. The ID Value is exposed as SlotName_ID variable. In addition to the single value it could be useful to have the nearest resolution always available.

For example:

"TramStation": {
  "name": "TramStation",
  "value": "hauptbahnhof ost",
  "resolutions": {
    "resolutionsPerAuthority": [
    {
        "authority": "amzn1.er-authority.echo-sdk.amzn1.ask.skill.91b2d64a-aaa7-4390-bb66-a196e950d8ea.TramStation",
        "status": {
          "code": "ER_SUCCESS_MATCH"
        },
        "values": [
          {
            "value": {
              "name": "Hauptbahnhof Ost",
              "id": "80029079"
            }
          },
          {
            "value": {
              "name": "Hauptbahnhof West",
              "id": "80029080"
            }
          }
        ]
      }
    ]
  },
  "confirmationStatus": "NONE",
  "source": "USER"
}

In that case I wouldn't get the correct resolution, as it's more than one. So it defaults to the spoken value. However the first resolution is always the one that fits best. For that reason I exposed two more variables named "SlotName_NEAREST" and "SlotName_NEAREST_ID". That way people can use it when they need it and can't rely on the default spoken value.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #86629
  • This PR is related to issue: #86629
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

AzonInc avatar Jan 26 '23 11:01 AzonInc

Hi AzonInc

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant[bot] avatar Jan 26 '23 11:01 home-assistant[bot]

Hey there @home-assistant/cloud, @ochlocracy, @jbouwh, mind taking a look at this pull request as it has been labeled with an integration (alexa) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of alexa can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign alexa Removes the current integration label and assignees on the issue, add the integration domain after the command.

home-assistant[bot] avatar Jan 26 '23 11:01 home-assistant[bot]

Please help me to understand why how this ID field would help. This is a part that I am not yet quite familiar with. It would help if you could explain what steps I need to make use of this.

jbouwh avatar Feb 07 '23 16:02 jbouwh

Please fix the errors, and tests. The PR code contains and breaks the alexa integration.

Sorry for that, I'm quite new and I'm not sure where these errors come from. But you can see that I didn't change any dependencies - just a small change. Unfortunately I have no Idea how to fix that.

I rebased to get up with the latest commits - dunno if it's fixed now due to some work in progress in dev branch before. Do you have an Idea what is causing that? Looks like this might be somewhat related.

AzonInc avatar Feb 07 '23 21:02 AzonInc

Please help me to understand why how this ID field would help. This is a part that I am not yet quite familiar with. It would help if you could explain what steps I need to make use of this.

The ID field would help whenever we want to pass some custom IDs (for example train Station Location IDs) to Homeassistant Automations (or Node-RED Addon Flows). It was initially removed because people thought there is no use because they can generate their entity ids by the name. However there are people out there, that dont want to use the Slot Value ID as entity ID, but as a real ID for TV Apps or Train Station IDs. That way it's not neccesary anymore to get the IDs somewhere else by Name if we could just serve them with Alexa. It's easier to work with the IDs rather than collecting them later or comparing strings that could have special characters.

I hope I could explain it.

AzonInc avatar Feb 07 '23 21:02 AzonInc

From what I see in the local tests they need to be fixed:

FAILED tests/components/alexa/test_intent.py::test_intent_request_with_slots - AssertionError: assert 500 == <HTTPStatus.OK: 200>
FAILED tests/components/alexa/test_intent.py::test_intent_request_with_slots_and_synonym_resolution - AssertionError: assert 500 == <HTTPStatus.OK: 200>
FAILED tests/components/alexa/test_intent.py::test_intent_request_with_slots_and_multi_synonym_resolution - AssertionError: assert 500 == <HTTPStatus.OK: 200>
FAILED tests/components/alexa/test_intent.py::test_intent_request_calling_service - AssertionError: assert 500 == <HTTPStatus.OK: 200>
FAILED tests/components/alexa/test_intent.py::test_intent_from_built_in_intent_library - AssertionError: assert 500 == <HTTPStatus.OK: 200>

Results (11.00s):
     182 passed
       5 failed
         - tests/components/alexa/test_intent.py:216 test_intent_request_with_slots
         - tests/components/alexa/test_intent.py:250 test_intent_request_with_slots_and_synonym_resolution
         - tests/components/alexa/test_intent.py:353 test_intent_request_with_slots_and_multi_synonym_resolution
         - tests/components/alexa/test_intent.py:481 test_intent_request_calling_service
         - tests/components/alexa/test_intent.py:553 test_intent_from_built_in_intent_library

The changed code seems not to be called during normal operation. So I do not really get when this code is used. Could you tell me step by step how to set up a test environment to be able to test/debug your code in real live.

jbouwh avatar Feb 08 '23 07:02 jbouwh

I finally figured how to properly run the Tests and Test Enviroment. All Tests are passing now. I added two more Tests to cover all scenarios of the new variables.

AzonInc avatar Feb 08 '23 13:02 AzonInc

I updated the branch to reflect the most recent changes to the tests file.

AzonInc avatar Feb 10 '23 19:02 AzonInc

Can you answer my questions on how this is supposed to be used and especially how I can test your code, and decide if this would fit or not. This might save you a lot of time,

jbouwh avatar Feb 10 '23 23:02 jbouwh

Scenareo: Ask Alexa for the next Tram to "Hauptbahnhof West" Ask Alexa for the next Tram to "Hauptbahnhof Ost" Ask Alexa for the next Tram to "Hauptbahnhof" (Synonym for West) Ask Alexa for the next Tram to "Bahnhof" (Synonym for West)

Problem:

  1. Currently multiple resolutions are not supported, even when the first resolution is always the correct one, the spoken value is used instead. As there are always multiple resolutions for everything that contains "Hauptbahnhof" it's failing to resolve "Hauptbahnhof Ost" and "Hauptbahnhof West" even when there is no synonym set. Because Amazon is sending the two but the correct resolution on index 0.
  2. Since the ID Support of the Alexa Integration got removed in the past, it's not possible to use the ID field of the Slot at all which is a pity.

Solution: I kept that behaviour with the Code changes and added three new variables. So there is no breaking change. One new Variable for the Slot ID: "SlotName"_ID Two new Variables for the nearest resolution value and id: "SlotName"_NEAREST, "SlotName"_NEAREST_ID


Alexa Intent Setup: image

Alexa Intent Slot Setup: image

Homeassistant Intent:

TramIntentTest:
  action:
    - service: input_text.set_value
      data:
        value: "{{ TramStation_NEAREST_ID }}"
      target:
        entity_id: input_text.tram_destination
    - service: button.press
      data: {}
      target:
        entity_id: button.request_tram
  speech:
    type: plain
    text: Wait a Second, I'm looking for a tram to {{ TramStation_NEAREST }}

Entities: Create one Input Button in HA with id: input_text.tram_destination Create a Button in NodeRED (Companion Integration) with id: button.request_tram


Now you can ask Alexa for the next Tram to some Station. The Intent is setting the input value to the nerest resolution tram station id and press the button in nodered to start the flow.

AzonInc avatar Feb 11 '23 11:02 AzonInc

I tried to setup a custom skill: using https://www.home-assistant.io/integrations/alexa.intent/#create-your-amazon-alexa-custom-skill (not successfully succeeded unfortunately ). Could that help to be able to use the intents as you demonstrate? Currently I only have a haaska skill set up for the common functionality. @balloob May be you could have a look at this proposal?

jbouwh avatar Feb 11 '23 12:02 jbouwh

I tried to setup a custom skill: using https://www.home-assistant.io/integrations/alexa.intent/#create-your-amazon-alexa-custom-skill (not successfully succeeded unfortunately ). Could that help to be able to use the intents as you demonstrate? Currently I only have a haaska skill set up for the common functionality. @balloob May be you could have a look at this proposal?

Yes. I use the HA Alexa Custom Skill for the TramStation Intent. However I don't think it's possible with HAASKA.

AzonInc avatar Feb 11 '23 12:02 AzonInc

I have successfully set up a test environment, but I have to pick this up later.

jbouwh avatar Feb 11 '23 16:02 jbouwh

I'm happy to hear. Just take your time and test whenever you're free :)

AzonInc avatar Feb 11 '23 16:02 AzonInc

Got an Hello world intent running, will try some more later to see what the ID field does.

jbouwh avatar Feb 13 '23 19:02 jbouwh

I uploaded My Homeassistant Skill JSON including the TramStationTest Intent: https://gist.github.com/AzonInc/c297f5642695bf4af0dfad4631b0f5a6

You can upload the model using the json editor: image

It will overwrite any existing intent configuration tho. It's invocation name is home assistant - feel free to change it or just play around with your own test intent :)

AzonInc avatar Feb 13 '23 19:02 AzonInc

Hi @jbouwh did you have time already to test the new intent slot variables?

AzonInc avatar Feb 25 '23 09:02 AzonInc

Hi @jbouwh did you have time already to test the new intent slot variables?

I have a working intent, but did not test with the code yet, may be I will next week?

jbouwh avatar Feb 25 '23 22:02 jbouwh

My intent works, but to trigger resolve_slot_data I need to have another HA intent with some shared intents.

jbouwh avatar Mar 07 '23 13:03 jbouwh

@jbouwh you can take the one of my Interaction model above as an example

AzonInc avatar Mar 07 '23 14:03 AzonInc

Okay, I think that I known now how this should work. I did not use node red, but created a light where the colors were the slot values, and that worked. I think the change is okay as it adds some additional variables for template rendering. Could you make a documentation PR and rebase this PR?

jbouwh avatar Mar 07 '23 18:03 jbouwh

Set the PR to ready for review when you are ready.

jbouwh avatar Mar 07 '23 18:03 jbouwh

@AzonInc do you need help?

jbouwh avatar Mar 20 '23 06:03 jbouwh

@AzonInc do you need help?

Hey, sorry for the delay - I have some trouble at the moment but I will look into it ASAP.

AzonInc avatar Mar 20 '23 18:03 AzonInc

@AzonInc do you need help?

Hey, I'm back. How can I make a documentation contribution? @jbouwh Where's the best place in the Documentation? I guess you mean this one: https://github.com/home-assistant/home-assistant.io/blob/current/source/_integrations/alexa.intent.markdown

AzonInc avatar May 04 '23 09:05 AzonInc

@AzonInc do you need help?

Hey, I'm back. How can I make a documentation contribution? @jbouwh Where's the best place in the Documentation? I guess you mean this one: https://github.com/home-assistant/home-assistant.io/blob/current/source/_integrations/alexa.intent.markdown

Correct, target against the next branch

jbouwh avatar May 04 '23 11:05 jbouwh

I created a docs PR: https://github.com/home-assistant/home-assistant.io/pull/27326 About that one missing type hint - If you have time and want to find out the type I'd be happy about that.

AzonInc avatar May 08 '23 12:05 AzonInc

Actually it could be way easier with only that one new Slot ID variable and a behaviour change which would lead to a breaking change tho.

I don't know why it is useful to prevent using the Slot resolutions when there is more than one possible resolution. But maybe you can explain me the reasons behind the decision.

Let's say we have an Intent Configuration like this: Utterance: Show me a route to {{ TrainStation}} Slot Name: TrainStation Slot Values: South Station, West Station, Apple Street and Junior Street

Behavior of the current implementation:

  • When there is no or more than one resolution, fall back to spoken Text

If I say "Alexa, show me a route to (West Street | West Station | Apple Street | Junior Street)", it will never use any of the resolutions bacause amazon is resolving them all as there are two values ending with street and two starting with west. So with the current implementation it will always fall back to the spoken Text in my example.

Suggestion: New implementation

  • The nearest resolution will always be used, even when there is more than one.
  • Only fall back to the spoken Text if there is no resolution available.

As far as I have seen and tested, the first resolution has always been the correct one. The new implementation however will be a breaking change for some people using the Slots as the resolved values might be a little bit different sometimes e.g. lowercase/uppercase/whitespace.

Let me know what you think about it.

AzonInc avatar May 09 '23 21:05 AzonInc

Actually it could be way easier with only that one new Slot ID variable and a behaviour change which would lead to a breaking change tho.

I don't know why it is useful to prevent using the Slot resolutions when there is more than one possible resolution. But maybe you can explain me the reasons behind the decision.

Let's say we have an Intent Configuration like this: Utterance: Show me a route to {{ TrainStation}} Slot Name: TrainStation Slot Values: South Station, West Station, Apple Street and Junior Street

Behavior of the current implementation:

* When there is no or more than one resolution, fall back to spoken Text

If I say "Alexa, show me a route to (West Street | West Station | Apple Street | Junior Street)", it will never use any of the resolutions bacause amazon is resolving them all as there are two values ending with street and two starting with west. So with the current implementation it will always fall back to the spoken Text in my example.

Suggestion: New implementation

* The nearest resolution will always be used, even when there is more than one.

* Only fall back to the spoken Text if there is no resolution available.

As far as I have seen and tested, the first resolution has always been the correct one. The new implementation however will be a breaking change for some people using the Slots as the resolved values might be a little bit different sometimes e.g. lowercase/uppercase/whitespace.

Let me know what you think about it.

I have invest some time to understand what you are suggestion and what would break. If we break anything it would be nice if we could have some backwards compatibility and a deprecation warning (repair). Don't know if that is possible though. If we can explain well enough what cases will break we could also explain this in the breaking changes release notes.

jbouwh avatar May 10 '23 06:05 jbouwh

btw: i see that the merging of dev pushed a lot of non related commits. to solve this you can do:

  • git fetch upstream
  • git rebase -i upstream/dev (select the commits you want to keep, or omit the -i flag)
  • git push --force So do not pull in dev in your branch.

jbouwh avatar May 10 '23 06:05 jbouwh