-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
SecGPT - LlamaIndex Integration #13127
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Hey @Yuhao-W,
I notice that we haven't used the standard process for creating a llama pack here. Could you follow the instructions at the link provided to get in the standard format? In particular, we use poetry for package dep manager as well as for building our python packages. For convenience we have a cli tool that helps you created these packs:
@Yuhao-W submitted a PR to your fork/main branch. It brings in the necessary pants build files to pass our checks. |
…ld-files add pants build files
@Yuhao-W looks like lint/fmt checks are failing. Can you please run:
|
@nerdai Hi, Andrei. I see that some checks failed. Is there anything that needs to be changed? |
Hey @Yuhao-W sorry for the troubles. I took a look at the logs and couldn't find anything. Tagging @logan-markewich who is quite good at figuring out this stuff when it seems like all is lost. lol |
I think I figured out the errors and updated the package by mainly including dependency information in the pyproject.toml file under our package path. I also set up a unit test environment and ran it locally. The unit test was passed on my end. @nerdai and/or @logan-markewich, would appreciate if you can review the changes! |
thanks, lets run the checks and see what happens! |
Thanks @andrei, it failed again : ( this time because the I have made a new commit. Not sure but we may still see errors after, would appreciate a deeper look if it fails. Thank you! |
@logan-markewich we're still running into some errors here. Perhaps we need to add a dependency in pants? This is the error we're seeing in the tests:
But (CC @Yuhao-W) |
@Yuhao-W got checks to pass 🥳. Needed to remove the requirements.txt file as it was tripping up pants having deps listed in both requirements.txt and pyproject.toml |
@@ -0,0 +1,741 @@ | |||
{ |
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'm a bit confused as to how these tools actually provide the fare price of both ride-sharing apps? Is this purely for illustration? In other words, is this notebook not actually functional?
Reply via ReviewNB
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.
You’re right! We developed these tools for attack illustration purposes. Developers can follow our examples to include their own real tools in SecGPT.
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.
Okay thanks for the confirmation -- perhaps just leaving a small comment in your notebook on that might make things more clear for your readers
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.
nvm i saw you added the comment with "simulated"
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.
what would be very great for a future PR is to have notebooks that are fully functional, i.e., examples that are completely repeateable
@@ -0,0 +1,741 @@ | |||
{ |
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.
Is there a way to show the case when not using SecGPT or having these measures turned off? In other words, can we see the case when the attack is successful?
Reply via ReviewNB
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.
That is a good idea! I have included another notebook, which defines a non-isolated LLM-based system to showcase the attack.
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.
awesome -- thanks!
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.
@Yuhao-W Thanks for this contribution! I'm really excited about this :)
I left some comments on your PR. As another blanket comment, I do think your pack would greatly improve if you were able to include some doc/class strings throughout your code (i.e., quick descriptions of funcs/classes and its params/args).
from llama_index.core import ChatPromptTemplate | ||
|
||
|
||
class HubPlanner: |
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.
Since there is a prompt here, I think we should subclass PromptMixin
:
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 have refined the prompt definition with a subclass of PromptMixin.
lc_output_parser = JsonOutputParser() | ||
self.output_parser = LangchainOutputParser(lc_output_parser) | ||
|
||
self.query_engine = QueryPipeline( |
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.
this may be a bit of a name clash with llama-index ecosystem. As this is not really a QueryEngine
but rather a QueryPipeline
. If possible, would suggest using a different name: query_pipeline
.
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 have revised accordingly.
|
||
|
||
class Message: | ||
def function_probe_request(self, spoke_id, function): |
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.
suggestion: maybe should this be a staticmethod?
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.
similarly for all other funcs?
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 have revised accordingly.
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.
Out of curiosity: have you considered using Pydantic BaseModel to represent Message? You can then subclass a BaseMessage. Pydantic can be helpful for validaton.
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.
That is a good idea. While I think the current representation of messages works, messages can also be represented using Pydantic BaseModel. However, as it requires non-trivial manual efforts, we can consider it in our future revisions.
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.
sgtm
llama-index-packs/llama-index-packs-secgpt/llama_index/packs/secgpt/spoke.py
Show resolved
Hide resolved
from llama_index.core.tools import FunctionTool | ||
|
||
|
||
def add_numbers(x: int, y: int) -> int: | ||
""" | ||
Adds the two numbers together and returns the result. | ||
""" | ||
return x + y | ||
|
||
|
||
if __name__ == "__main__": | ||
llm = OpenAI(model="gpt-4-turbo", temperature=0.0, additional_kwargs={"seed": 0}) | ||
function_tool = FunctionTool.from_defaults(fn=add_numbers) | ||
print(function_tool.metadata) | ||
print(function_tool.metadata.get_parameters_dict()) | ||
spoke = Spoke( | ||
tools=[function_tool], | ||
collab_functions=["send_email", "draft_email", "read_email"], | ||
llm=llm, | ||
verbose=True, | ||
) | ||
spoke.chat("send a email to [email protected], subject: hello, body: hello world") |
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.
This looks like it was used perhaps for testing while developing? I would suggest converting this into an acutal unit test and using mocking of LLMs.
For inspiration, see here: https://github.com/run-llama/llama_index/blob/main/llama-index-integrations/agent/llama-index-agent-introspective/tests/test_self_reflection.py
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.
Thanks for pointing it out. he code is used for temporally testing. I have removed it.
# Format and send the app request message to the hub | ||
def make_request(self, functionality: str, request: dict): | ||
# format the app request message | ||
app_request_message = Message().app_request( |
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.
if these are staticmethods then you should be able to do Message.app_request(...)
instead.
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 have revised accordingly.
llama-index-packs/llama-index-packs-secgpt/llama_index/packs/secgpt/hub.py
Outdated
Show resolved
Hide resolved
@nerdai Thanks for the feedback on the PR! I will address your comments and get back to you with an update soon. |
hey @Yuhao-W: how's this coming along? |
Hi @nerdai . Thanks for your suggestions. I have addressed all your comments and included doc/class strings for all classes and functions. |
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.
Thanks @Yuhao-W for making the changes. I think things look good. The only thing I am wondering about is if we can have a small fully functional example of SecGPT working? I think the notebooks are only for illustrations (simulations) and we don't really have and unit tests.
In other words, I cannot be sure that this package will work as intended. Can we add a small fully functional (even, toy) example?
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.
sgtm
llama-index-packs/llama-index-packs-secgpt/llama_index/packs/secgpt/spoke.py
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.
Thanks @Yuhao-W -- this was a big one! Very excited to have this merged.
Description
SecGPT is an LLM-based system that secures the execution of LLM apps via isolation. The key idea behind SecGPT is to isolate the execution of apps and to allow interaction between apps and the system only through well-defined interfaces with user permission. SecGPT can defend against multiple types of attacks, including app compromise, data stealing, inadvertent data exposure, and uncontrolled system alteration. We develop SecGPT using LlamaIndex because it supports several LLMs and apps and can be easily extended to include additional LLMs and apps. We implement SecGPT as a personal assistant chatbot, which the users can communicate with using text messages.
New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Suggested Checklist:
make format; make lint
to appease the lint gods