-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 support for HTML, CSS and Javascript in LocalCommandLineCodeExecutor #2303
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2303 +/- ##
===========================================
+ Coverage 38.14% 67.02% +28.88%
===========================================
Files 78 78
Lines 7865 7879 +14
Branches 1683 1824 +141
===========================================
+ Hits 3000 5281 +2281
+ Misses 4615 2118 -2497
- Partials 250 480 +230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
who requested this feature? Could you add them as reviewers? |
else: | ||
try: | ||
program = _cmd(lang) | ||
except NotImplementedError: |
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 is confusing - some languages silently get saved and ignored and some don't? Ignoring NotImplementedError
doesn't feel right
save_code_only
feels way too coarse to me. I would rather see a mapping of language to whether it should be executed or just saved.
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.
Agreed. A mapping to specify whether a given language should be executed is more suitable here. In that case we can allow user to specify the executing program as well, bringing more transparency and getting rid of _cmd.
I am working on an example that creates a web application using AutoGen GroupChat. Supporting HTML, Javascript and CSS in code executor is required to address this scenario, as we don't want to execute those code blocks. Add several contributors that has been working on software engineering related usage cases. |
Pull request was converted to draft
Converted to draft as this needs more work. Will be back on this next week. |
@ekzhu You know, I'm just using tool for that ;) For me chenges are looking good, we will don't need to define tool for file saving any more. Maybe we should extend that feature for other file formats, for example .vue? |
This PR is for LocalCommandLineCodeExecutor and util.py , can we translate the support for docker_commandline_code_executor ? |
program = sys.executable | ||
else: | ||
try: | ||
program = _cmd(lang) |
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.
there can implement like this:
- put this using function _cmd to instance method, so user can overwrite it
- _cmd can return None,means there is no need to execute
program = self.get_lang_cmd(lang)
if program is None:
logs_all += f"Code saved to {str(written_file)}\n"
exitcode = 0
continue
I am swamped currently. Would you like to continue the work in this PR to address your issue #2379? You can take a look at the @jackgerrits 's comment earlier #2303 (comment) regarding how to parameterize the executor for how to execute specific languages. You can fork from this branch to create your own PR. Then I can close this one. |
I re-branch your branch and then I created an empty commit but I was denied. |
@asandez1 hmm, can you fork the Microsoft repo to your own account, and then checkout this PR's branch. After you commit your changes, push to a new branch in your own fork. Then, create a PR to merge the branch on your own fork to the main branch in the Microsoft repo. |
Close as the work is continued in #2464 |
…utor with Mapping executor/saver #2303 (#2464) * Add support for HTML, CSS and Javascript in LocalCommandLineCodeExecutor * init branch * init branch * feat: test code execution added * fix: test update * fix: test * fix: policy test * feat: default policy --------- Co-authored-by: Eric Zhu <[email protected]>
…utor with Mapping executor/saver microsoft#2303 (microsoft#2464) * Add support for HTML, CSS and Javascript in LocalCommandLineCodeExecutor * init branch * init branch * feat: test code execution added * fix: test update * fix: test * fix: policy test * feat: default policy --------- Co-authored-by: Eric Zhu <[email protected]>
Why are these changes needed?
Many web application development use cases require support for simply saving the static content like HTML, CSS and Javascript.
This PR adds support for those languages in LocalCommandLineCodeExecutor.
It also add option in LocalCommandLineCodeExecutor for only saving the code files not executing them. This is useful when the web application code starts a server and never exits.
Related issue number
Closes #2379 #2383
Checks