-
Notifications
You must be signed in to change notification settings - Fork 164
feat: Add GitHub integration with agent_prompts and github_components #1637
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
Conversation
@sjrl I added a test called https://colab.research.google.com/drive/1ktlwQ-CDLGDs2uZXvzgG8XspfjPidYqZ?usp=sharing @sjrl If GitHubFileEditorTool looks good to you, I will add tools for all other components and probably update the directory structure a bit. |
@julian-risch This is related to the change we made to tools to have a new variable called ...
outputs_to_state={
#"message": {"source": "documents", "handler": message_handler}, TODO
"documents": {"source": "documents"},
},
outputs_to_string={"source": "documents", "handler": message_handler}
... |
@sjrl Finally ready for another review! We're using What do you think about the parameter name |
:param name: Optional name for the tool. | ||
:param description: Optional description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do realize this is a bit confusing in our tools, but it seems that if we define a __init__
then these docstrings are put under the __init__
def. If there is no __init__
defined like in Tool
then we put it in the class description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, of course! Do you think we should a usage example here then (in addition to moving the param docstrings to the init)? I realized that's missing too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if it's not too much to ask, a usage example would be great!
integrations/github/src/haystack_integrations/tools/github/issue_commenter_tool.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Only some minor comments left
Related Issues
Related to Add GitHub integration with agent_prompts and github_components #1650
Related to Move Agent from experimental to main haystack-experimental#250
@sjrl brought up that one way to keep example files from the experimental Agent around is an integration chore: Remove Agent after Haystack 2.12 release haystack-experimental#263 (comment)
Proposed Changes:
The idea is to enable users to run the example notebook (or a version with updated imports) after having installed this new integration.
How did you test it?
New unit tests and I ran all usage examples successfully with a test repo.
I have tested it with the updated notebook. I'll update the cookbook PR once the integration is released: deepset-ai/haystack-cookbook#183
Notes for the reviewer
github_token
parameter toapi_key
for consistency with many other integrations.GithubRepositoryViewer has abranch
parameter in the run method, which could also be named ref to make more clear it can also be a tag or commit hash. I prefer keeping the parameter namebranch
.Some components havegithub_token: Optional[Secret] = None,
because they can work without any token while others useSecret.from_env_var("GITHUB_TOKEN")
. I suggest we useSecret.from_env_var("GITHUB_TOKEN", strict=False)
where we currently haveNone
as the default.The internal implementation of the components differs in how they use_get_headers
or_get_request_headers
or define headers inline. We could refactor that.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.