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

Python interpreter #617

Merged
merged 16 commits into from
Aug 13, 2024
Merged

Python interpreter #617

merged 16 commits into from
Aug 13, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 13, 2024

  • Modified BuiltinTools.mdx and system.mdx to add documentation for a new feature python_interpreter. This tool allows execution of Python 3.12 code in a docker container. 🐍
  • Updated cli/src/docker.ts and cli/src/run.ts for improved management of Docker containers. The changes include functions to stop Docker containers, better error handling, and verbose logging. 🐳
  • Updated chat completion response to include messages in core/src/chat.ts.
  • Added new Python based tool python_interpreter in core/src/genaisrc/system.python_interpreter.genai.js that allows executing Python code in a Docker container.
  • Refined CSVToMarkdown function in core/src/csv.ts to generate more compact markdown that improves LLM performance. 📈
  • Updated core/src/liner.test.ts for more comprehensive testing.
  • Some tweaks in fetch request tracing on core/src/fetch.ts.
  • Added new Python code samples in sample/genaisrc/data-analyst.genai.mjs and sample/genaisrc/pandas.genai.mjs.
  • Updated core/src/prompt_template.d.ts for a new function stop to stop and clean Docker containers. 👨‍💻
  • Multiple changes for better error logging and outputs. 🚀

generated by pr-describe

@@ -10,6 +10,7 @@ import { LinkCard } from '@astrojs/starlight/components';
<LinkCard title="fs_read_file" description="Reads a file as text from the file system." href="/genaiscript/reference/scripts/system#systemfs_read_file" />
<LinkCard title="fs_read_summary" description="Reads a summary of a file from the file system." href="/genaiscript/reference/scripts/system#systemfs_read_summary" />
<LinkCard title="math_eval" description="Evaluates a math expression" href="/genaiscript/reference/scripts/system#systemmath" />
<LinkCard title="python_interpreter" description="Executes python 3.12 code in a docker container. The process output is returned. Use 'print' to output data." href="/genaiscript/reference/scripts/system#systempython_interpreter" />

Choose a reason for hiding this comment

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

The link provided for the 'python_interpreter' tool should be consistent with other links, using 'system' as the prefix in the href attribute.

generated by pr-docs-review-commit link_format

type: "string",
description: `list of pip packages to install using pip. should be using the pip install format:
<package1>
<package2>

Choose a reason for hiding this comment

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

The code block for the 'requirements' description should be formatted as a code snippet for clarity.

generated by pr-docs-review-commit code_format

console.log(`installing: ` + requirements)
await container.writeText(
"requirements.txt",
requirements.replace(/[ ,]\s*/g, "\n")

Choose a reason for hiding this comment

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

The regular expression pattern used in the 'requirements.txt' replacement should escape the period to match a literal period, not any character.

generated by pr-docs-review-commit regex_pattern

await container.writeText("main.py", main)
const res = await container.exec("python", ["main.py"])
return res
}

Choose a reason for hiding this comment

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

The 'defTool' function is defined as an async function but does not use 'await' with the 'host.container' call, which is likely an asynchronous operation.

generated by pr-docs-review-commit async_function

}
this.containers.splice(i, 1)
}
}

Choose a reason for hiding this comment

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

Missing error handling in the stopContainer method. There are empty catch blocks which could swallow errors and make debugging difficult.

generated by pr-review-commit missing_error_handling

@@ -130,7 +124,7 @@ export async function runScript(
const ffs = await host.findFiles(arg, {
applyGitIgnore: excludeGitIgnore,
})
if (!ffs.length) {
if (!ffs?.length) {

Choose a reason for hiding this comment

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

Potential null reference exception. The ffs variable is not checked for null before accessing its length property.

generated by pr-review-commit missing_null_check

: undefined
trace.itemValue(`args`, callArgs ?? call.arguments)
trace.fence(call.arguments, "json")
if (callArgs === undefined) trace.error("arguments failed to parse")

Choose a reason for hiding this comment

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

Missing error handling. If callArgs is undefined, an error is logged but execution continues which could lead to unexpected behavior.

generated by pr-review-commit missing_error_handling

Copy link

The pull request contains several changes:

  1. In docker.ts, a new method stopContainer is introduced to stop and remove a specific container by id. This could be useful for cleanup operations. Additionally, more verbose logging has been added, which would be helpful for troubleshooting.

  2. In run.ts, the use of progress spinners has been removed, and instead, verbose logging is being used. This could make the output cleaner and easier to read, especially for CI/CD pipelines.

  3. In chat.ts, added error logging to tool call and improved arguments parsing to better handle markdown fenced code blocks.

  4. In csv.test.ts, several new tests were added to cover more functionality of CSV parsing and conversion to markdown. This would improve overall test coverage and confidence in the CSV functionality.

  5. In csv.ts, adjustments were made to the options passed to the markdownTable function used in CSVToMarkdown. This may have effects on how the generated markdown tables are formatted.

  6. In fetch.ts, a minor change was made in the traceFetchPost function that replaces carriage return and line feed characters with a newline and a backslash. This could affect how the traced fetch post commands are formatted.

  7. In json5.ts, a new function JSONLLMTryParse was added to handle markdown fenced code blocks while parsing JSON.

  8. In liner.test.ts, error logging was added to assist with diagnosing test failures.

  9. In promptrunner.ts and types/prompt_template.d.ts, changes were made to handle container cleanup operations.

However, there seems to be a potential issue:

  1. In expander.ts, it's not clear why messages length is checked to determine the status of the expansion.

Overall, these changes seem to be beneficial in improving the debugging experience and test coverage. The removal of the spinning progress indicator in favor of logs might make the output less visually appealing, but it should be more useful in an automated environment, such as CI/CD pipelines. The addition of verbose logging should also assist with debugging. Without further context, it's hard to say if the potential issue would cause any real problems. The changes seem well-implemented and should be positive overall.

Thus, my response is:

LGTM 🚀

Please consider reviewing the potential issue in expander.ts to ensure it doesn't cause unexpected behavior.

generated by pr-review

description: `list of pip packages to install using pip. should be using the pip install format:
<package1>
<package2>
`,

Choose a reason for hiding this comment

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

The description for 'requirements' property in the 'python_interpreter' tool is incorrectly formatted. It should be a single line description without line breaks.

generated by pr-docs-review-commit description_format

await container.writeText("main.py", main)
const res = await container.exec("python", ["main.py"])
return res
}

Choose a reason for hiding this comment

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

The 'defTool' function is defined as 'async' but does not use 'await' within its body, which is likely an error.

generated by pr-docs-review-commit async_usage

title: "Python Dockerized code execution",
})

const image = env.vars.pythonImage ?? "python:3.12"

Choose a reason for hiding this comment

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

The usage of 'env.vars.pythonImage' is not explained or documented, which could lead to confusion about where this variable is set or how to use it.

generated by pr-docs-review-commit env_usage

@@ -130,7 +124,7 @@ export async function runScript(
const ffs = await host.findFiles(arg, {
applyGitIgnore: excludeGitIgnore,
})
if (!ffs.length) {
if (!ffs?.length) {
return fail(
`no files matching ${arg}`,
FILES_NOT_FOUND_ERROR_CODE

Choose a reason for hiding this comment

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

Missing error handling for the case when ffs is undefined. It's recommended to handle this case to prevent potential runtime errors.

generated by pr-review-commit missing_error_handling

await writeFile(hostFilename, content, { encoding: "utf8" })
await writeFile(hostFilename, content ?? "", {
encoding: "utf8",
})

Choose a reason for hiding this comment

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

Potential null reference error. The content parameter is not checked for null or undefined before being used in writeFile.

generated by pr-review-commit missing_null_check

@pelikhan pelikhan merged commit 4bdb161 into main Aug 13, 2024
10 checks passed
@pelikhan pelikhan deleted the python-interpreter branch August 13, 2024 18:53
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.

1 participant