Add custom tool headers #2773

pablonyx · 2024-10-12T01:50:57Z

Description

[Provide a brief description of the changes in this PR]

How Has This Been Tested?

[Describe the tests you ran to verify your changes]

Accepted Risk

[Any know risks or failure modes to point out to reviewers]

Related Issue(s)

[If applicable, link to the issue(s) this PR addresses]

Checklist:

All of the automated tests pass
All PR comments are addressed and marked resolved
If there are migrations, they have been rebased to latest main
If there are new dependencies, they are added to the requirements
If there are new environment variables, they are added to all of the deployment methods
If there are new APIs that don't require auth, they are added to PUBLIC_ENDPOINT_SPECS
Docker images build and basic functionalities work
Author has done a final read through of the PR right before merge

vercel · 2024-10-12T01:51:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
internal-search	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 5:34pm

greptile-apps

PR Summary

This pull request introduces custom tool headers functionality across the Danswer system, enhancing flexibility in API interactions and tool operations.

Added TOOL_PASS_THROUGH_HEADERS configuration in backend/danswer/configs/model_configs.py for managing custom tool headers
Implemented get_tool_headers function in new file backend/danswer/tools/headers.py to process and filter headers based on configuration
Modified CustomTool class in backend/danswer/tools/custom/custom_tool.py to incorporate tool_additional_headers in API requests
Updated handle_new_chat_message in backend/danswer/server/query_and_chat/chat_backend.py to include tool headers in chat processing
Added tool_additional_headers parameter to stream_chat_message_objects and stream_chat_message functions in backend/danswer/chat/process_message.py, though usage is not yet implemented

_{6 file(s) reviewed, 10 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-12T01:55:29Z

backend/danswer/chat/process_message.py

+    tool_additional_headers: dict[str, str] | None = None,
    is_connected: Callable[[], bool] | None = None,


logic: New parameter tool_additional_headers added but not used in the function body

greptile-apps · 2024-10-12T01:55:46Z

backend/danswer/configs/model_configs.py

@@ -119,3 +119,17 @@
        logger.error(
            "Failed to parse LITELLM_PASS_THROUGH_HEADERS, must be a valid JSON object"
        )
+
+
+TOOL_PASS_THROUGH_HEADERS: list[str] | None = None


style: Consider adding a comment explaining the purpose of TOOL_PASS_THROUGH_HEADERS

greptile-apps · 2024-10-12T01:55:46Z

backend/danswer/configs/model_configs.py

+_TOOL_PASS_THROUGH_HEADERS_RAW = os.environ.get("TOOL_PASS_THROUGH_HEADERS")
+if _TOOL_PASS_THROUGH_HEADERS_RAW:
+    try:
+        TOOL_PASS_THROUGH_HEADERS = json.loads(_TOOL_PASS_THROUGH_HEADERS_RAW)


logic: Ensure TOOL_PASS_THROUGH_HEADERS is a list of strings after parsing

backend/danswer/configs/model_configs.py

greptile-apps · 2024-10-12T01:56:43Z

backend/danswer/tools/custom/custom_tool.py

@@ -47,6 +47,7 @@ def __init__(
        method_spec: MethodSpec,
        base_url: str,
        custom_headers: list[dict[str, str]] | None = [],
+        tool_additional_headers: dict[str, str] | None = None,


style: Consider adding type hints for the tool_additional_headers parameter

greptile-apps · 2024-10-12T01:56:44Z

backend/danswer/tools/custom/custom_tool.py

@@ -59,6 +60,7 @@ def __init__(
            if custom_headers
            else {}
        )
+        self.headers.update(tool_additional_headers or {})


style: Use self.headers.update(tool_additional_headers or {}) for consistency with line 58

greptile-apps · 2024-10-12T01:56:44Z

backend/danswer/tools/custom/custom_tool.py

@@ -185,6 +187,7 @@ def final_result(self, *args: ToolResponse) -> JSON_ro:

 def build_custom_tools_from_openapi_schema_and_headers(
    openapi_schema: dict[str, Any],
+    tool_additional_headers: dict[str, str] | None = None,


style: Add a docstring explaining the purpose of tool_additional_headers

greptile-apps · 2024-10-12T01:56:57Z

backend/danswer/tools/headers.py

+        if key in headers:
+            pass_through_headers[key] = headers[key]
+        elif lowercase_key in headers:
+            pass_through_headers[lowercase_key] = headers[lowercase_key]


style: This logic might lead to unexpected behavior if both original and lowercase keys exist in headers. Consider prioritizing or documenting the behavior

we should write a docstring here to document what the inputs and outputs look like. then if we need to conform keys to lowercase to do the matching, then let's do that instead of matching twice, etc.

It's also unclear why this has to be an implementation specific function that knows what a "tool" is. If this is a function that passes through some headers based on a set of other headers, then perhaps we should write it that way with the appropriate params, and not access external state to make the function work.

Agree re: a docstring– believe that would help make this clear- we do need the external config to be tool specific because it's specifically for passing headers through to the custom tool calls

Understand the function is being used here for filtering tool headers, but it should be written more generically as an function of the form extract_headers(headers, TOOL_PASS_THROUGH_HEADERS) without accessing global state. This allows the function to be pure and gives us a utility function we can put somewhere in case we want to reuse it.

backend/danswer/configs/model_configs.py

rkuo-danswer · 2024-10-12T02:59:54Z

backend/danswer/tools/headers.py

+        if key in headers:
+            pass_through_headers[key] = headers[key]
+        elif lowercase_key in headers:
+            pass_through_headers[lowercase_key] = headers[lowercase_key]


we should write a docstring here to document what the inputs and outputs look like. then if we need to conform keys to lowercase to do the matching, then let's do that instead of matching twice, etc.

It's also unclear why this has to be an implementation specific function that knows what a "tool" is. If this is a function that passes through some headers based on a set of other headers, then perhaps we should write it that way with the appropriate params, and not access external state to make the function work.

rkuo-danswer · 2024-10-14T01:26:06Z

backend/danswer/tools/headers.py

+        if key in headers:
+            pass_through_headers[key] = headers[key]
+        elif lowercase_key in headers:
+            pass_through_headers[lowercase_key] = headers[lowercase_key]


Understand the function is being used here for filtering tool headers, but it should be written more generically as an function of the form extract_headers(headers, TOOL_PASS_THROUGH_HEADERS) without accessing global state. This allows the function to be pure and gives us a utility function we can put somewhere in case we want to reuse it.

vercel bot deployed to Preview October 12, 2024 01:52 View deployment

pablonyx marked this pull request as ready for review October 12, 2024 01:55

vercel bot deployed to Preview October 12, 2024 01:56 View deployment

greptile-apps bot reviewed Oct 12, 2024

View reviewed changes

vercel bot deployed to Preview October 12, 2024 02:25 View deployment

rkuo-danswer requested changes Oct 12, 2024

View reviewed changes

pablonyx force-pushed the tool_headers branch from 1a059c8 to 6d9e151 Compare October 12, 2024 17:57

pablonyx requested a review from rkuo-danswer October 12, 2024 17:58

vercel bot deployed to Preview October 12, 2024 17:58 View deployment

rkuo-danswer requested changes Oct 14, 2024

View reviewed changes

pablonyx added 6 commits October 15, 2024 10:31

add custom tool headers

6185899

simplify

df1b9a5

k

23d0765

k

d1f076b

k

745fcc8

nit

129d357

pablonyx force-pushed the tool_headers branch from 6d9e151 to 129d357 Compare October 15, 2024 17:31

pablonyx requested a review from rkuo-danswer October 15, 2024 17:33

vercel bot deployed to Preview October 15, 2024 17:34 View deployment

rkuo-danswer approved these changes Oct 16, 2024

View reviewed changes

rkuo-danswer added this pull request to the merge queue Oct 16, 2024

Merged via the queue into main with commit 11372aa Oct 16, 2024
7 checks passed

pablonyx deleted the tool_headers branch October 17, 2024 23:13

This was referenced Nov 27, 2024

chore/merge upstream 2024112601 mindvalley/danswer#79

Merged

chore/merge upstream 2024112801 mindvalley/danswer#81

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom tool headers #2773

Add custom tool headers #2773

pablonyx commented Oct 12, 2024

vercel bot commented Oct 12, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

greptile-apps bot Oct 12, 2024

rkuo-danswer Oct 12, 2024

pablonyx Oct 12, 2024 •

edited

Loading

rkuo-danswer Oct 14, 2024

rkuo-danswer Oct 12, 2024

rkuo-danswer Oct 14, 2024

		tool_additional_headers: dict[str, str] \| None = None,
		is_connected: Callable[[], bool] \| None = None,

Add custom tool headers #2773

Add custom tool headers #2773

Conversation

pablonyx commented Oct 12, 2024

Description

How Has This Been Tested?

Accepted Risk

Related Issue(s)

Checklist:

vercel bot commented Oct 12, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 12, 2024

Choose a reason for hiding this comment

rkuo-danswer Oct 12, 2024

Choose a reason for hiding this comment

pablonyx Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

rkuo-danswer Oct 14, 2024

Choose a reason for hiding this comment

rkuo-danswer Oct 12, 2024

Choose a reason for hiding this comment

rkuo-danswer Oct 14, 2024

Choose a reason for hiding this comment

vercel bot commented Oct 12, 2024 •

edited

Loading

pablonyx Oct 12, 2024 •

edited

Loading