microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.94k stars 3.27k forks source link

Python: recall method in StepwisePlan not using context #3237

Closed tarockey closed 8 months ago

tarockey commented 1 year ago

Describe the bug When using the StepwisePlanner, context is not being passed to skills. This is manifesting when using the recall ability of the TextMemorySkill in a skill's prompt, always using the default settings (e.g. collection name, limits, etc.). This means that skills cannot be context-dependent.

To Reproduce Steps to reproduce the behavior:

  1. Create a new Kernel, register the TextMemorySkill, and a memory store (currently tested with Azure Cognitive Search as memory store)
  2. Create a skill which uses the {{recall}} method inside the prompt template, and register to the kernel.
  3. Define a new context with the name of your ACS index as the COLLECTION_PARAM for TextMemorySkill (as shown below):
    context = kernel.create_new_context()
    context[sk.core_skills.TextMemorySkill.COLLECTION_PARAM] = memory_collection_name
    context[sk.core_skills.TextMemorySkill.RELEVANCE_PARAM] = 0.5
    context[sk.core_skills.TextMemorySkill.LIMIT_PARAM] = 5
  4. Create a new SequentialPlanner, passing in your kernel, and a question as the goal.
  5. Invoke(invoke_async) your plan, passing in the context and memory as parameters:
    result = await plan.invoke_async(context=context, memory=kernel.memory)
  6. Review results, noting that if your ACS index is not named "generic", it will fail to retrieve the relevant memories.
  7. Enumerate through the plan._steps, and note the outputs. You will likely see the error
    Error occurred: Function `recall` execution failed. ResourceNotFoundError: () The index 'generic' for service 'YOUR ACS INDEX' was not found.

Expected behavior The recall function should have been passed the correct collection (index) name, and returned relevant memories for the prompt to use as additional context.

Platform

Additional Info

Full repro code below:

import semantic_kernel as sk
from dotenv import load_dotenv
from semantic_kernel.connectors.ai.open_ai import AzureChatCompletion, AzureTextEmbedding
from requests import get
import json
import os
from Plugins.HAPlugin.Homeassistant import HomeAssistant
from semantic_kernel.planning import StepwisePlanner
from semantic_kernel.planning.stepwise_planner.stepwise_planner_config import (
    StepwisePlannerConfig,
)
from semantic_kernel.connectors.memory.azure_cognitive_search import AzureCognitiveSearchMemoryStore

load_dotenv()

kernel = sk.Kernel()

deployment, api_key, endpoint = sk.azure_openai_settings_from_dot_env()

kernel.add_chat_service("chat_completion", AzureChatCompletion(deployment, endpoint, api_key))
intentskills = kernel.import_semantic_skill_from_directory("./skills", 'IntentDetectionSkill')
kernel.add_text_embedding_generation_service("embedding", AzureTextEmbedding("embedding", endpoint, api_key))

kernel.register_memory_store(memory_store=AzureCognitiveSearchMemoryStore(search_endpoint=os.environ.get('AZURE_COGNITIVE_SEARCH_URL'), admin_key=os.environ.get("AZURE_COGNITIVE_SEARCH_APIKEY"), vector_size=1536))

memory_collection_name = 'hadocs'
kernel.import_skill(sk.core_skills.TextMemorySkill())

ask = "Can HomeAssistant split energy usage by Tarriff?"

planner = StepwisePlanner(
    kernel, StepwisePlannerConfig(max_iterations=10, min_iteration_time_ms=1000)
)

plan = planner.create_plan(goal=ask)

context = kernel.create_new_context()
context[sk.core_skills.TextMemorySkill.COLLECTION_PARAM] = memory_collection_name
context[sk.core_skills.TextMemorySkill.RELEVANCE_PARAM] = 0.5
context[sk.core_skills.TextMemorySkill.LIMIT_PARAM] = 5

result = await plan.invoke_async(context=context, memory=kernel.memory)
print(result)

for index, step in enumerate(plan._steps):
    print("Step:", index)
    print("Description:",step.description)
    print("Function:", step.skill_name + "." + step._function.name)
    if len(step._outputs) > 0:
        print( "  Output:\n", str.replace(result[step._outputs[0]],"\n", "\n  "))

Sample config.json


{
    "schema": 1,
    "description": "Answer questions related to the home automation system based on documentation",
    "type": "completion",
    "completion": {
      "max_tokens": 100,
      "temperature": 0.1,
      "top_p": 1.0,
      "presence_penalty": 0.0,
      "frequency_penalty": 0.0
    }
  }

skprompt.txt

If a user has a DocsQuestion, answer the question to the best of your ability using the memory context provided to you.
You are to be helpful and polite. If your memory does not have a relevant answer, do not make up an answer, instead say "I don't know".

context: {{recall $input}}

Please cite your answers with the description property of the memory, and the additional_metadata (listed as "headers")

For example:

Question: Which rasbperry pi hats work with z-wave?

Answer:     - Aeotec Z-Pi 7 Raspberry Pi HAT/Shield (ZWA025, 700 series)
            - Z-Wave.Me RaZberry 7 (ZME_RAZBERRY7, 700 series)
            - Z-Wave.Me RaZberry 7 Pro (ZMEERAZBERRY7_PRO or ZMEURAZBERRY7_PRO, 700 series)
            - Z-Wave.Me Razberry 2 (500 series)"

            source: "Extended instructions how to setup Z-Wave."
            headers: ["## Supported Z-Wave USB Sticks & Hardware Modules"]

Question: How do I edit my configuration.yaml file?

Answer: The easiest option to edit `configuration.yaml` is to use the {% my supervisor_addon title="Studio Code Server add-on" addon="a0d7b954_vscode" %}.

            source: "Configuring Home Assistant via text files."
            headers: ["Editing `configuration.yaml`"]

Question: {{$input}}

Answer:
tarockey commented 1 year ago

Additional info: I tried instead to use the SequentialPlanner skill, defining my plan with

planner = SequentialPlanner(
    kernel=kernel,
    config=SequentialPlannerConfig(max_relevant_functions=10),
)
plan = await planner.create_plan_async(goal=ask)

This also isn't working for me - the input is always showing as an empty string, which of course then fails to be parsed. I tried passing the input with the plan.invoke_async(context=context, memory=kernel.memory, input=ask, but that didn't change anything. Am I just using the SDK wrong?

evchaki commented 1 year ago

@tarockey - thanks for the details here, the team will take a look and see what is happening.

evchaki commented 1 year ago

@RogerBarreto could you see what is going on here?

MaxMelcher commented 1 year ago

I have the same issue in dotnet.

tarockey commented 11 months ago

Any updates on this bug @evchaki ?

evchaki commented 11 months ago

@moonbox3 could you take a look here.

moonbox3 commented 11 months ago

@evchaki, @tarockey let me work on repro'ing the issue.

kalyankdas commented 11 months ago

Any update on this issue on python side? Need a fix, seems like I cannot call any native function from semantic prompt,

moonbox3 commented 11 months ago

Hi @kalyankdas, apologies for dropping the ball on this. I'm going to assign this to @juliomenendez who should be able to dive in soon. We'll update you as soon as we can.

juliomenendez commented 11 months ago

Thanks @moonbox3, I'll take a first look over the weekend. Will keep this issue up to date as soon as I can.

kalyankdas commented 11 months ago

To add more context, :) with latest python package - Sample is broken too , where it uses recall,(https://github.com/microsoft/semantic-kernel/tree/08e2e033c5c6c7f4490fcc311bc4730288d88c69/python/notebooks) /06-memory-and-embeddings.ipynb

Also, kernel.import_skill(sk.core_skills.TextMemorySkill()) some how it does not work either

moonbox3 commented 11 months ago

@kalyankdas I'm trying to get a new release out that contains some fixes for the TextMemorySkill (how they context variables are defined).

Something faster to try: run poetry install if you haven't. Once you do that, and all dependencies are installed, comment out the notebook's first cell (to install the package from pip). You can then run all other cells and it will use the underlying code. I can fully run the 06-memory-and-embeddings notebook.

moonbox3 commented 11 months ago

@kalyankdas I published a new version of the python SDK (0.4.3.dev0). The notebooks were updated to reflect this, so if you install the package via pip like the notebook shows, the TextMemorySkill errors shouldn't be there.

juliomenendez commented 11 months ago

Hey @kalyankdas @tarockey I have a WIP PR with a fix for calling skills from StepwisePlanner. On my branch and using the provided code from the issue description I get this result:

Yes, it is possible to split energy usage by tariff in HomeAssistant using the utility_meter component. The component allows for the creation of multiple meters that track energy usage based on different tariffs. If the utility_meter component is not yet configured in HomeAssistant, we can use the save function to add the necessary configuration to the configuration.yaml file.
Step: 0
Description: Execute a plan
Function: StepwisePlanner.ExecutePlan
  It is possible to split energy usage by tariff in HomeAssistant using the utility_meter component. This component allows for the creation of multiple meters that track energy usage based on different tariffs. We can use the recall function to check if the utility_meter component is already configured in HomeAssistant. If it is not, we can use the save function to add the necessary configuration to the configuration.yaml file.
  [ACTION]
  {"action": "_GLOBAL_FUNCTIONS_.recall", "action_variables": {"input": "utility_meter component"}}
  [OBSERVATION]
  Got no result from action
  [THOUGHT]
  Since we did not get any result from the recall function, it is likely that the utility_meter component is not yet configured in HomeAssistant. We can use the save function to add the necessary configuration to the configuration.yaml file. We will need to specify the input as the configuration for the utility_meter component, and provide a unique key to associate with this configuration.
  [ACTION]
  {"action": "_GLOBAL_FUNCTIONS_.save", "action_variables": {"input": "utility_meter:\n  electricity:\n    source: sensor.energy\n    cycle: daily\n    tariffs:\n      peak:\n        cost: 0.25\n        peak: '07:00-19:00'\n      offpeak:\n        cost: 0.10\n        peak: '19:00-07:00'", "key": "utility_meter_config"}}
  [OBSERVATION]
  utility_meter:
    electricity:
      source: sensor.energy
      cycle: daily
      tariffs:
        peak:
          cost: 0.25
          peak: '07:00-19:00'
        offpeak:
          cost: 0.10
          peak: '19:00-07:00'
  [THOUGHT]
  None

Is that what you are expecting? If not, how should the correct response look?

tarockey commented 11 months ago

@juliomenendez , that possibly looks correct, but without an ACS index to successfully retrieve from recall, it's hard to tell? Do you have an example you could run where you are connecting to your ACS index, even if it's just a toy sample (rather than having to ingest HomeAssistant docs?)

that said, I'm seeing issues in the SequentialPlanner as well (on the latest public release, not your branch) also related to context, and not passing variables, so the fact that each action step has an "input" being populated is promising.

juliomenendez commented 11 months ago

@tarockey Using this script:

import os
import semantic_kernel as sk
from dotenv import load_dotenv
from semantic_kernel.connectors.ai.open_ai import AzureChatCompletion, AzureTextEmbedding
from semantic_kernel.planning import StepwisePlanner
from semantic_kernel.planning.stepwise_planner.stepwise_planner_config import (
    StepwisePlannerConfig,
)
from semantic_kernel.connectors.memory.azure_cognitive_search import AzureCognitiveSearchMemoryStore
from semantic_kernel.utils.settings import azure_openai_settings_from_dot_env_as_dict

memory_collection_name = 'generic'

async def populate_memory(kernel: sk.Kernel):
    await kernel.memory.save_information_async(collection=memory_collection_name, id="info1", text="""The Rasbperry pi hats that work with z-wave are:
- Aeotec Z-Pi 7 Raspberry Pi HAT/Shield (ZWA025, 700 series)
- Z-Wave.Me RaZberry 7 (ZME_RAZBERRY7, 700 series)
- Z-Wave.Me RaZberry 7 Pro (ZMEERAZBERRY7_PRO or ZMEURAZBERRY7_PRO, 700 series)
- Z-Wave.Me Razberry 2 (500 series)""")
    await kernel.memory.save_information_async(collection=memory_collection_name, id="info2", text="The easiest option to edit `configuration.yaml` is to use the Studio Code Server add-on")

async def main():
    load_dotenv()

    kernel = sk.Kernel()

    aoai_settings_dict = azure_openai_settings_from_dot_env_as_dict(include_deployment=True, include_api_version=True)

    kernel.add_chat_service("chat_completion", AzureChatCompletion(**aoai_settings_dict))
    kernel.add_text_embedding_generation_service("ada", AzureTextEmbedding("embedding", endpoint=aoai_settings_dict["endpoint"], api_key=aoai_settings_dict["api_key"]))

    memory = AzureCognitiveSearchMemoryStore(search_endpoint=os.environ.get('AZURE_AISEARCH_URL'), admin_key=os.environ.get("AZURE_AISEARCH_API_KEY"), vector_size=1536)
    kernel.register_memory_store(memory_store=memory)

    kernel.import_skill(sk.core_skills.TextMemorySkill())

    await populate_memory(kernel)

    ask = "Can HomeAssistant split energy usage by Tarriff?"

    planner = StepwisePlanner(
        kernel, StepwisePlannerConfig(max_iterations=10, min_iteration_time_ms=1000)
    )
    plan = planner.create_plan(ask)

    context = kernel.create_new_context()
    context[sk.core_skills.TextMemorySkill.COLLECTION_PARAM] = memory_collection_name
    context[sk.core_skills.TextMemorySkill.RELEVANCE_PARAM] = 0.5
    context[sk.core_skills.TextMemorySkill.LIMIT_PARAM] = 5

    result = await plan.invoke_async(context=context, memory=kernel.memory)
    print(result)

    for index, step in enumerate(plan._steps):
        print("Step:", index)
        print("Description:",step.description)
        print("Function:", step.skill_name + "." + step._function.name)
        if len(step._outputs) > 0:
            print( "  Output:\n", str.replace(result[step._outputs[0]],"\n", "\n  "))

    await memory.close_async()

if __name__ == "__main__":
    import asyncio

    asyncio.run(main())

I get the following output:

Yes, it is possible to split energy usage by tariff in HomeAssistant using the utility_meter component. The component allows for the creation of multiple meters that track energy usage based on different tariffs. To configure the utility_meter component, we can use the save function to add the necessary configuration to the configuration.yaml file. The configuration should include the necessary meters for tracking energy usage based on different tariffs.
Step: 0
Description: Execute a plan
Function: StepwisePlanner.ExecutePlan
  Output:
 This was my previous work (but they haven't seen any of it! They only see what I return as final answer):
  [THOUGHT]
  It is possible to split energy usage by tariff in HomeAssistant using the utility_meter component. This component allows for the creation of multiple meters that track energy usage based on different tariffs. We can use the recall function to check if the utility_meter component is already configured in HomeAssistant. If it is not, we can use the save function to add the necessary configuration to the configuration.yaml file.
  [ACTION]
  {"action": "_GLOBAL_FUNCTIONS_.recall", "action_variables": {"input": "utility_meter component"}}
  [OBSERVATION]
  ["The easiest option to edit `configuration.yaml` is to use the Studio Code Server add-on"]
  [THOUGHT]
  Since the recall function did not return any relevant information, we can assume that the utility_meter component is not yet configured in HomeAssistant. We can use the save function to add the necessary configuration to the configuration.yaml file. We will need to specify the input as the configuration for the utility_meter component, and provide a unique key to associate with this configuration. We can use the default collection value of "generic".
  [ACTION]
  {"action": "_GLOBAL_FUNCTIONS_.save", "action_variables": {"input": "utility_meter:\n  gas_daily:\n    source: sensor.gas_meter\n    cycle: daily\n  gas_night:\n    source: sensor.gas_meter\n    cycle: daily\n    offset:\n      hours: 21\n  electricity_daily:\n    source: sensor.electricity_meter\n    cycle: daily\n  electricity_night:\n    source: sensor.electricity_meter\n    cycle: daily\n    offset:\n      hours: 21", "key": "utility_meter_config"}}
  [OBSERVATION]
  utility_meter:
    gas_daily:
      source: sensor.gas_meter
      cycle: daily
    gas_night:
      source: sensor.gas_meter
      cycle: daily
      offset:
        hours: 21
    electricity_daily:
      source: sensor.electricity_meter
      cycle: daily
    electricity_night:
      source: sensor.electricity_meter
      cycle: daily
      offset:
        hours: 21
  [THOUGHT]
  None

It looks to me that it is finding the stored information in ACS. I do see your point of the ACS index having to be named "generic" but we can talk about that separately if possible.

juliomenendez commented 11 months ago

@tarockey my PR has been merged. Can you check this issue again? Check the sample script I provided in my previous comment where ACS is being used.

tarockey commented 9 months ago

@juliomenendez apologies, I am just now seeing this comment, I will try it out ASAP, thank you!

tarockey commented 8 months ago

Apologies for the late response - after testing locally, I still seem to be having issues getting the memory to be used as context for an answer. Additionally, I cannot get the service to recognize an index that isn't named 'generic'. That said, this is all on an old version of semantic kernel, I haven't had chance to refactor to the latest versions (currently on 0.4.7.dev0). For now, it probably makes sense to close this issue since its so stale, and I can reopen if I see it again in a newer version.