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

Add CodeNode #992

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add CodeNode #992

wants to merge 5 commits into from

Conversation

proteusvacuum
Copy link
Collaborator

Description

Runs arbitrary python within the pipeline.
It essentially is running a function:

def main(input):
    {The Code Goes Here}

The return value of the function is used as the output.

The main reason I encapsulated it inside of a separate function is that you can then:

  • define other functions e.g.
def foo(bar):
    return bar + 1

return foo(3)
  • clearly see what is being returned with a return statement

I still need to add: a code widget (is there a preference? I was thinking of using ace as I think that is what is used in HQ)

For now, this doesn't access "shared state" that we had also spoken about, but I wanted to get this out there for discussion first.

User Impact

Demo

Screencast.from.2024-12-13.15-03-02.mp4

Docs

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 98.92473% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
apps/pipelines/tests/utils.py 75.00% 1 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

Copy link
Collaborator

@snopoke snopoke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great.

In terms of an editor I've been wanting to get going with https://codemirror.net/

@proteusvacuum
Copy link
Collaborator Author

Screencast.from.2024-12-13.22-42-34.mp4

@proteusvacuum
Copy link
Collaborator Author

proteusvacuum commented Dec 14, 2024

Currently the python is extremely restricted. As we overwrite __builtins__ we need to redefine things like _inplacevar_ in a safe way, like this:

https://github.com/zopefoundation/AccessControl/blob/f9ae58816f0712eb6ea97459b4ccafbf4662d9db/src/AccessControl/ZopeGuards.py#L625

There are other potentially safe functions that we could add, e.g:
https://github.com/ndurner/amz_bedrock_chat/blob/830e496bcc0e6fa8e8b6e361c5419dd53d5d72e6/code_exec.py#L41-L99

This can happen in a followup, I think the basic functionality is enough to get going with.

@proteusvacuum proteusvacuum marked this pull request as ready for review December 14, 2024 04:11
("return f'Hello, {input}!'", "World", "Hello, World!"),
("", "foo", "foo"), # No code just returns the input
(EXTRA_FUNCTION, "blah", "other blah"), # Calling a separate function is possible
("'foo'", "", "None"), # No return value will return "None"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should instead raise an exception in this case, as it was probably not intentional to not return a string.

@proteusvacuum
Copy link
Collaborator Author

Made a few updates after @bderenzi gave some feedback on slack.

Now the main function is exposed to the user, and there is a bit more information provided for syntax errors, etc.

Screencast.from.2024-12-14.21-26-28.mp4

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

Successfully merging this pull request may close these issues.

3 participants