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

Python: Cannot import native functions to the Kernel that have more than one parameter. #624

Closed
alexchaomander opened this issue Apr 24, 2023 · 3 comments
Assignees

Comments

@alexchaomander
Copy link
Contributor

Describe the bug
Right now, when you try to import a native function to the Semantic Kernel that has more than one parameter, you run into this issue

  File "/Users/jtohahn/Desktop/GPT/semantic-kernel/python/commodore/commodore.py", line 29, in <module>
    kernel.import_skill(FileIOSkill(), "file")
  File "/Users/jtohahn/Desktop/GPT/semantic-kernel/.venv/lib/python3.10/site-packages/semantic_kernel/kernel.py", line 223, in import_skill
    SKFunction.from_native_method(candidate, skill_name, self.logger)
  File "/Users/jtohahn/Desktop/GPT/semantic-kernel/.venv/lib/python3.10/site-packages/semantic_kernel/orchestration/sk_function.py", line 80, in from_native_method
    delegate_type=DelegateInference.infer_delegate_type(method),
  File "/Users/jtohahn/Desktop/GPT/semantic-kernel/.venv/lib/python3.10/site-packages/semantic_kernel/orchestration/delegate_inference.py", line 243, in infer_delegate_type
    if wrapped(function_signature, awaitable):
TypeError: DelegateInference.infer_unknown() takes 1 positional argument but 2 were given

To Reproduce
Example function:

@sk_function(description="List the contents of a directory", name="listDirectory")
    async def list_directory(
        self, input_dir: str, depth: int = 0, parent_idx: str = ""
    ) -> str:
        """
        List the contents of a directory

        Example:
            {{file.listDirectory $path }}
        Args:
            input_dir -- The path to the directory to list

        Returns:
            A representation of the directory structure
        """

        assert os.path.exists(input_dir), f"Directory {input_dir} does not exist"

        tree_str = ""
        if not os.path.isdir(input_dir):
            raise ValueError(f"{input_dir} is not a directory")

        for idx, entry in enumerate(os.listdir(input_dir)):
            path = os.path.join(input_dir, entry)
            current_idx = f"{parent_idx}.{idx}" if parent_idx else f"{idx}"
            if os.path.isfile(path):
                tree_str += f"F{current_idx}:{entry}|"
            elif os.path.isdir(path):
                tree_str += f"D{current_idx}:{entry}|"
                tree_str += self.list_directory(path, depth + 1, current_idx)

        return tree_str[:-1]

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. Windows]
  • IDE: [e.g. Visual Studio, VS Code]
  • NuGet Package Version [e.g. 0.1.0]

Additional context
Add any other context about the problem here.

@joowon-dm-snu
Copy link
Contributor

joowon-dm-snu commented Apr 25, 2023

@alexchaomander

I met that issue last night too! If I understand semantic-kernel correctly, the list_directory not working is by design.
you can find its design here semantic-kernel/python/semantic_kernel/orchestration/delegate_inference.py

There can be at most two parameters, and only str or SKContext can be used as NativeFunction's parameter and there are a limited number of possible combinations.
list_directory requires 3 paramters (str, str, str), which is why the error occurs.

Anyway I opened the PR separately because I thought it was an inappropriate warning.

I think it would be more usable NativeFunction if I could put in multiple parameters like you wrote :)

@alexchaomander
Copy link
Contributor Author

This might be helpful and if so should be communicated more clearly:

image

@microsoftShannon
Copy link
Contributor

@dluc We should look to have better error messaging in Python. Can we add this to the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants