-
Notifications
You must be signed in to change notification settings - Fork 942
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
Feat: Added command center #2674
base: main
Are you sure you want to change the base?
Feat: Added command center #2674
Conversation
Performance benchmarks:
|
Some known limitations:
|
This is super cool. I'm in a long distance train this afternoon, I will try to look at it then! |
I think this is cool. However, the unchecked use of I don't have time to dig a bit deeper, but e.g., https://www.stackhawk.com/blog/command-injection-python/ might be a good starting point to think about this a bit more. |
for more information, see https://pre-commit.ci
@EwoutH I don't think it is possible to import custom classes directly, but it works if the additional imports are passed to the to |
I suggest also checking the code module, which allows you to attach a full console to a running python process. |
…r/mesa into feat-command-center
for more information, see https://pre-commit.ci
I did some research on making |
The |
Thanks for checking! Might it be an idea to open a separate discussion or issue where you share what you did? One dream feature would be to have a full variable explorer and console when running mesa models through the GUI. This PR is a stepping stone towards that vision and anything you learned can be valuable for others. I'll try to review this PR more closely hopefully over the weekend. |
That sounds like a great feature! I have opened an issue (#2683) for discussions. I decided to start from scratch to make it easier to incorporate valuable input. I would greatly appreciate any help you can provide! |
@@ -157,6 +164,8 @@ def SolaraViz( | |||
) | |||
with solara.Card("Information"): | |||
ShowSteps(model.value) | |||
with solara.Card("Command Center"): |
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.
do we want the command center to be allways there, or make it optional in some way?
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 personally would prefer it to be always be there, it will encourage the users to play around with it and know it better.
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.
From a design point of view, I prefer giving users the choice but show it in all our examples.
Someone might want to built a UI with very limited possibility for interaction, or only show the animation to a client.
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.
Makes sense, does a boolean parameter in SolaraViz
work?
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 was thinking more along the lines of treating it as a component so you could just at it to the list of components:
page = SolaraViz(
model,
components=[SpaceComponent, SomeGraphComponent, Console],
model_params=model_params,
name="my fancy model",
)
But I am open to having it as a boolean as well. I haven't thought about the layout anyway in great detail.
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, that works as well, in fact that is more intuitive. I like this idea more than that of a boolean.
@@ -586,3 +595,156 @@ def ShowSteps(model): | |||
"""Display the current step of the model.""" | |||
update_counter.get() | |||
return solara.Text(f"Step: {model.steps}") | |||
|
|||
|
|||
class _SecureCodeValidator(ast.NodeVisitor): |
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 did you use as inspiration, if any for this?
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 didn't find any source exactly resembling this, but after search a bit I found some open discussions on stackoverflow suggesting the use of ast
, after doing a little research over it, I used AI to assemble those ideas into a prototype.
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.
Should we not move all command center stuff into its own seperate module?
A few clarifying questions to start this review.
I realized the same after implementing it, but I think the development of console is going smooth, that can replace the command center entirely. We can have an separate file for that. |
I just want to say that I am really impressed with the work done here and in #2683 . Definitely something that has been on my frontend wishlist for a long time. But I am not sure how to proceed from here, is this PR here superseded by the work done in #2683? Or should this here be merged before implementing the changes there? Or is the scope of both features different? |
Thanks @Corvince for your reply!
Yes the work done in #2683 will replace the work done by this PR. This PR is here to get the review on the the feature |
"open", # File system access | ||
"globals", | ||
"locals", # Global state manipulation | ||
} |
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.
most of these make sense to me
is the model itself by default available in the namespace?
also, we can refine this based on experiences
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.
Sorry, I don't think I exactly understand what you mean here. Can you add a little more context.
f"Access to system module '{attr}' is not allowed" | ||
) | ||
|
||
self.generic_visit(node) |
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 is the overhead of this security validation?
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 ran some tests, and most of the time, the overhead from the security validator is more than double the execution time of the code.
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 lot, it might be an argument to leave out validation at all and just give a very clear warning in the docs about security.
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 it's a lot, I didn't think it to be so performance heavy. But since it could be used for smaller models, I think a warning in the docs should do the job (personal view).
I am no expert in this domain, but I reviewed your code with the help of an ai assistant. While your code offers some first protection against malicious actors, it is by no means complete. There are various ways to circumvent the protections. So I think its more of an architectural problem of how and where the code is executed. If you run this yourself on your local computer, I see no additional security risk and I don't think we need a code validator. If the model is hosted some site where the code is executed client-side (e.g. py.cafe, basically the same applies, you can only harm yourself (at most). This leaves only the possibility of hosting it on a server, where the code is also executed on the server. In principle this is always a bad idea, no matter the amount of protection. Something like this can only work safely by whitelisting allowed commands, not trying to catch malicious attempts. So in summary I don't think we need the code validator. For server environments it might even give a false impression of security. We could warn not to use the command center for hosted solutions, but since we don't currently have any guidelines for that anyways its a bit pointless. Don't get me wrong, enabling easy hosting of models should be high on our todo list imho, but it should be run client-side, not server-side anyways. |
Although I am sympathetic to @Corvince's points, I don't fully agree. One use case is that I am giving a model with a UI to some novice student. She will run the model locally, but might not be intimately familiar with python or even with programming. In such a context, a minimum level of protection of what can be done via the console seems desirable. |
Good point, you are right, I haven't considered the use-case of someone who isn't a model developer, but is playing with a model locally. But I am not sure how much of a risk that is. Its not that easy to cause serious harm. If you are worried about that you shouldn't let users use the computer unsupervised ;) |
Summary
This feature is inspired by this comment from issue #2176.
This pull request aims to add a command center into Solara Viz to execute commands directly from the browser to your model.
Implementation
This PR adds a solara component named

CommandCenter
into the sidebar directly. Runs commands written in the text area directly in the global scope usingexec
function and updates the visualization to show the changes.Every function and property available through model can be used and updated using the command center. Some examples:
model.step()
print(len(model.agents))
For custom imports:
now both
np
andmyAgent
can be used in the command center.