Optionally run Codejail in a external service using REST API. #27795

ericfab179 · 2021-06-01T14:25:08Z

Add codejail service settings (endpoint and feature flag).
Add conditional to allow running codejail using a REST API service when flag is enabled.

openedx-webhooks · 2021-06-01T14:25:13Z

Thanks for the pull request, @ericfab179! I've created OSPR-5820 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2021-06-01T20:01:35Z

@ericfab179 Thank you for your contribution. Please let me know once it is ready for our review.

jfavellar90 · 2021-06-04T18:18:28Z

common/lib/capa/capa/safe_exec/safe_exec.py

@@ -6,10 +6,12 @@
 from codejail.safe_exec import SafeExecException, json_safe
 from codejail.safe_exec import not_safe_exec as codejail_not_safe_exec
 from codejail.safe_exec import safe_exec as codejail_safe_exec
+from django.conf import settings


This import is not used in the file

jfavellar90 · 2021-06-04T18:24:05Z

common/lib/capa/capa/safe_exec/safe_exec.py

+        }
+        response_globals_dict, emsg = send_safe_exec_request(data, extra_files)
+
+        if emsg:


I think this logic of creating the exception and updating the globals should be performed by send_safe_exec_request method

jfavellar90 · 2021-06-04T18:29:27Z

common/lib/capa/capa/safe_exec/helpers.py

+def send_safe_exec_request(data, extra_files):
+    codejail_service_endpoint = get_codejail_rest_service_endpoint()
+    payload = json.dumps(data)
+    response = requests.post(


What about wrapping the requests call with a try-catch, similar to https://github.com/edx/edx-platform/blob/master/lms/djangoapps/edxnotes/helpers.py#L105. Notice that in the notes example, a custom exception is raised, a custom one could be used here as well since this is a critical component that impacts the user experience.

jfavellar90 · 2021-06-04T18:34:59Z

common/lib/capa/capa/safe_exec/helpers.py

+    payload = json.dumps(data)
+    response = requests.post(
+        codejail_service_endpoint,
+        files=extra_files,


This is just to have a discussion: in case of Codejail service API failure, should the platform fall back to the local execution (at least during the adoption of the remote service)?

jfavellar90 · 2021-06-04T21:39:26Z

common/lib/capa/capa/safe_exec/helpers.py

+def send_safe_exec_request(data, extra_files):
+    codejail_service_endpoint = get_codejail_rest_service_endpoint()
+    payload = json.dumps(data)
+    response = requests.post(


I consider we should set a timeout for these requests, which could be configurable

mariajgrimaldi · 2021-06-15T20:22:01Z

common/lib/capa/capa/safe_exec/exceptions.py

+    """
+    An exception that is raised whenever Codejail service is unavailable.
+    """
+    pass  # lint-amnesty, pylint: disable=unnecessary-pass


why is this necessary?

@mariajgrimaldi We are basically copying the edxnotes structure https://github.com/edx/edx-platform/blob/master/lms/djangoapps/edxnotes/exceptions.py

These lint-amnesty comments were added by a tool that allowed us to ignore existing lint failures and prevent new ones from coming in -- if you're copying code from somewhere, you'll want to resolve the lint failure instead.

In this case, you can actually get rid of the entire line, both pass and linter comment. The pass is apparently unneeded because the comment serves as the body of the class (this surprised me, but OK!) and that's what the linter was complaining about. I suspect that when the original code was written in notes, the comments weren't there at first, so the author added pass to wrap up the class and then forgot to remove it later. :-)

pdpinch · 2021-07-09T17:33:06Z

Hi @ericfab179, we're really curious about this PR and the possibility of a codejail as a service. What are your plans for this?

jfavellar90 · 2021-07-14T16:20:13Z

common/lib/capa/capa/safe_exec/helpers.py

+
+        response_json = response.json()
+
+        emsg = response_json["emsg"]


Safer way to get the error message.

Suggested change

emsg = response_json["emsg"]

emsg = response_json.get("emsg")

jfavellar90 · 2021-07-14T16:23:35Z

common/lib/capa/capa/safe_exec/helpers.py

+
+            exception = SafeExecException(emsg)
+
+        globals_dict.update(response_json["globals_dict"])


Suggested change

globals_dict.update(response_json["globals_dict"])

globals_dict.update(response_json.get("globals_dict"))

Indeed, what happens if the response does not contain a "globals_dict" key? Maybe validation is required to make sure "globals_dict" is returned and it contains proper data

jfavellar90 · 2021-07-14T16:30:28Z

common/lib/capa/capa/safe_exec/helpers.py

+
+        globals_dict.update(response_json["globals_dict"])
+
+    except RequestException:


Is this exception imported? Does it throw an exception when the codejail service sends backs a HTTP status different to 2xx?

ericfab179 · 2021-07-22T01:55:05Z

Hi @ericfab179, we're really curious about this PR and the possibility of a codejail as a service. What are your plans for this?

Hi @pdpinch we are in the process of internal reviews with the edunext team and as soon as we are done with that we are going to let you know for the edx review. This PR is related to this other two repositories:

Tutor plugin for Codejail: https://github.com/eduNEXT/tutor-contrib-codejail/pull/1/files
Codejail service: https://github.com/eduNEXT/codejailservice/pull/2/files
I will also let you know when those PRs are ready for review.

nedbat · 2021-07-29T18:22:54Z

@ericfab179 why is this pull request against Koa? Typically we make changes on master, and then backport them as needed.

ormsbee · 2021-08-01T20:07:43Z

Just noticed this in my notifications. @timmc-edx and some others at edX were doing some investigation around this as well during a hackathon a while back. Have you connected with him or seen the notes from that hackathon's investigation by any chance?

ormsbee · 2021-08-01T20:11:33Z

One of the things I'm personally curious about from the design point of view is how much the startup costs are, and how that might be mitigated. We sometimes call codejail dozens of times during a single request, and I'd be concerned if each one of those has an expensive initialization process.

If it's too early to ask those kinds of questions, I can definitely wait. I'm very excited that you folks are pursuing this. Thank you.

common/lib/capa/capa/safe_exec/helpers.py

ericfab179 · 2021-08-06T14:26:49Z

One of the things I'm personally curious about from the design point of view is how much the startup costs are, and how that might be mitigated. We sometimes call codejail dozens of times during a single request, and I'd be concerned if each one of those has an expensive initialization process.

If it's too early to ask those kinds of questions, I can definitely wait. I'm very excited that you folks are pursuing this. Thank you.

Hi @ormsbee thanks for your comment. Not sure if you are thinking about running every codejail execution in a dedicated container created on-demand. I think @regisb tried that but as you mentioned the initialization times were not good enough for our use of case. So now we are thinking about handling codejail as a service like notes or discovery, which run independently to edx-platform, which allows us to have better control over it. You can see more about the discussion of codejai integration with tutor here overhangio/tutor#284. However we still need to think about scalability, which probably would need to be connected some how to edx-platform. The discussion is open and your opinion/feedback would be truly appreciated.

ericfab179 · 2021-08-06T14:28:21Z

@natabene This PR is now ready for review. Thanks for your help.

CC @regisb @BbrSofiane

pdpinch · 2021-08-11T18:18:54Z

@ericfab179 we're really interested in trying this out, in particular with the mitx-grading-library and its sample course that exercises a lot of custom-response features -- in particular exception handling.

It's not clear to me how to run the service that goes with this PR. Do I have to run it in Tutor, or is there a way to run it in Devstack?

felipemontoya · 2021-10-06T20:35:09Z

common/lib/capa/capa/safe_exec/helpers.py

@@ -0,0 +1,92 @@
+"""
+Helper methods related to safe exec.


@ericfab179 could we call this file remote or remote_exec to give a better idea of what kinds of helpers we should expect here?

felipemontoya · 2021-10-06T20:47:46Z

common/lib/capa/capa/safe_exec/safe_exec.py

+            "unsafely": unsafely
+        }
+
+        emsg, exception = send_safe_exec_request(data, extra_files)


since we are already talking about a next version of this service that might cache the extra_files I think I'd like to raise a point here. It does not have to be blocking given that we also want this for maple so that tutor installs can use codejail.

We could call this version send_safe_exec_request_v0 and make the calling here configurable.
Then this _v0 function will be tied to the v0 url path in the remote API call.

Once we start upgrading the service and the call here to use v1 with the cache and whatnot we dont need to necessarily break the usage for those that do not want to install a more complicated codejail service or simply for those that need to upgrade step by step and might need to leave the v0 service running for a bit longer.

If we want to make it easy, I would say we only need to make one function signature for v0 and subsequent, for instance by adding the extra_files to the data dict and pass just that. Also we need to rely not only in is_codejail_rest_service_enabled() but also one function that does get_remote_exec function that returns send_safe_exec_request_v0 for now. Once we are ready to extend we only need to extend the remote or helpers module to add support for v1 while v0 will still be usable.

ericfab179 · 2021-10-09T16:52:37Z

@felipemontoya I tried to address your comments could you please check the PR. Also I tested it again and built new images. I tried to test with master but I was having problems when creating users related to lang_pref. So I decided to back-port the changes to lilac (here is the branch that I used https://github.com/eduNEXT/edx-platform/tree/ednx/lilac-codejailservice). It is working as expected I also solved some problems that I found during the testing.
If anyone is interested in testing you need to set the following variables:

tutor config save \
--set DOCKER_IMAGE_OPENEDX=docker.io/ednxops/edxapp:tutor.lilac.2ade0549badbd264497cb144041a0a56e1d32170\
--set DOCKER_IMAGE_OPENEDX_DEV=docker.io/ednxops/edxapp:tutor.lilac.dev.2ade0549badbd264497cb144041a0a56e1d32170 \
--set EDX_PLATFORM_REPOSITORY=https://github.com/eduNEXT/edx-platform.git \
--set EDX_PLATFORM_VERSION=ednx/lilac-codejailservice \
--set SANDBOX_PYTHON_VERSION=3.5.10

I used Tutor v12.1.3 and you can follow same instructions as given here https://github.com/edx/edx-platform/pull/27795#issuecomment-904795672
BTW @SergeyLuksha it should solve the problems you were facing.

felipemontoya · 2021-10-13T18:00:45Z

Thanks a lot @ericfab179. With the latest round of changes it looks great for me too.

I did not quite understand why you had to backport it to lilac. Was it only to be able to build tutor images to test? If so, then that should not be a blocker for merging.

felipemontoya · 2021-10-13T18:02:08Z

@ericfab179 you only need to solve the quality error:

common/lib/capa/capa/safe_exec/remote_exec.py:37: [C0116(missing-function-docstring), get_remote_exec] Missing function or method docstring

And we should be ready with this.

regisb

I really like what I see here in this PR. It's simple and straight to the point. My only comment would concern the lack of documentation for the CODE_JAIL_* settings -- as you know I'm a big fan of documenting stuff in Open edX. But my comment should probably not be a blocker for this PR.

Add codejail service settings (endpoint and feature flag). Add conditional to allow running codejail using a REST API service when flag is enabled.

Address recommendations and fixes discussed in PR. Use similar structure to notes app. Fix errors detected in tests.

@timmc-edx

Address fixes recommended by @timmc-edx.

Address @felipemonotoya recommendations related to add versioning to safe_exec_remote functions.

nltk 3.6.3 drops support for python 3.5.

edx-status-bot · 2021-10-15T13:47:34Z

Your PR has finished running tests. There were no failures.

felipemontoya · 2021-10-15T14:35:24Z

@regisb we added annotations to the settings so that it is better documented from the start.

openedx-webhooks · 2021-10-15T14:36:19Z

@ericfab179 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2021-10-15T15:42:48Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-10-15T16:29:51Z

EdX Release Notice: This PR has been deployed to the production environment.

…ernal service using REST API.

@nedbat

…nal service  ## Description This is a backport of https://github.com/edx/edx-platform/pull/27795. The idea of this PR is to make the external Codejail service feature available in the Maple release, which will help to integrate Codejail with the Tutor ecosystem. From openedx/wg-build-test-release#98 (comment) we can confirm the feature is working. ## Deadline "None".

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jun 1, 2021

ericfab179 force-pushed the eric/codejail_rest_service branch from c415246 to ea761b6 Compare June 3, 2021 21:55

jfavellar90 reviewed Jun 4, 2021

View reviewed changes

ericfab179 force-pushed the eric/codejail_rest_service branch 2 times, most recently from 609ab16 to 0773b83 Compare June 7, 2021 22:14

mariajgrimaldi reviewed Jun 15, 2021

View reviewed changes

jfavellar90 reviewed Jul 14, 2021

View reviewed changes

ericfab179 force-pushed the eric/codejail_rest_service branch from 1440b01 to 21a8712 Compare August 1, 2021 14:53

ericfab179 changed the base branch from open-release/koa.master to master August 1, 2021 14:54

ericfab179 force-pushed the eric/codejail_rest_service branch 3 times, most recently from 0adec99 to f458150 Compare August 1, 2021 19:37

jfavellar90 reviewed Aug 3, 2021

View reviewed changes

common/lib/capa/capa/safe_exec/helpers.py Outdated Show resolved Hide resolved

ericfab179 marked this pull request as ready for review August 6, 2021 14:27

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Aug 6, 2021

ericfab179 mentioned this pull request Aug 9, 2021

[Tutor] Codejail plugin should be ready for Maple openedx/wg-build-test-release#98

Closed

openedx-webhooks added the awaiting prioritization label Aug 9, 2021

felipemontoya reviewed Oct 6, 2021

View reviewed changes

ericfab179 force-pushed the eric/codejail_rest_service branch 4 times, most recently from bd69904 to d8f6991 Compare October 9, 2021 16:46

ericfab179 force-pushed the eric/codejail_rest_service branch from d8f6991 to fee6761 Compare October 9, 2021 17:01

felipemontoya approved these changes Oct 13, 2021

View reviewed changes

regisb reviewed Oct 14, 2021

View reviewed changes

ericfab179 added 3 commits October 14, 2021 08:11

feat: Codejail as external service (optional)

3a736ee

Add codejail service settings (endpoint and feature flag). Add conditional to allow running codejail using a REST API service when flag is enabled.

fix: Improve and fix codejail service integration

d672d30

Address recommendations and fixes discussed in PR. Use similar structure to notes app. Fix errors detected in tests.

fix: Address PR comments

fc7104c

Address fixes recommended by @timmc-edx.

ericfab179 force-pushed the eric/codejail_rest_service branch from fee6761 to 67a8074 Compare October 14, 2021 13:14

ericfab179 added 2 commits October 15, 2021 08:19

fix: Address more PR comments

fe68828

Address @felipemonotoya recommendations related to add versioning to safe_exec_remote functions.

fix: Constrain nltk version for py35 sandbox

2a371e8

nltk 3.6.3 drops support for python 3.5.

ericfab179 force-pushed the eric/codejail_rest_service branch from 67a8074 to 2a371e8 Compare October 15, 2021 13:20

felipemontoya merged commit 2100537 into openedx:master Oct 15, 2021

openedx-webhooks added merged and removed changes requested labels Oct 15, 2021

regisb mentioned this pull request Oct 26, 2021

Minor compatibility fixes eduNEXT/tutor-contrib-codejail#2

Merged

jfavellar90 added a commit to eduNEXT/edx-platform that referenced this pull request Oct 28, 2021

chore: backport of PR openedx#27795, Optionally run Codejail in a ext…

a703bb8

…ernal service using REST API.

jfavellar90 mentioned this pull request Oct 28, 2021

PR #27795 backport, Optionally run Codejail in a external service #29161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally run Codejail in a external service using REST API. #27795

Optionally run Codejail in a external service using REST API. #27795

ericfab179 commented Jun 1, 2021

openedx-webhooks commented Jun 1, 2021 •

edited

Loading

natabene commented Jun 1, 2021

jfavellar90 Jun 4, 2021

jfavellar90 Jun 4, 2021

jfavellar90 Jun 4, 2021

jfavellar90 Jun 4, 2021

jfavellar90 Jun 4, 2021

mariajgrimaldi Jun 15, 2021

ericfab179 Jun 16, 2021

timmc-edx Sep 22, 2021

pdpinch commented Jul 9, 2021

jfavellar90 Jul 14, 2021

jfavellar90 Jul 14, 2021

jfavellar90 Jul 14, 2021

ericfab179 commented Jul 22, 2021

nedbat commented Jul 29, 2021

ormsbee commented Aug 1, 2021

ormsbee commented Aug 1, 2021

ericfab179 commented Aug 6, 2021

ericfab179 commented Aug 6, 2021

pdpinch commented Aug 11, 2021

felipemontoya Oct 6, 2021

felipemontoya Oct 6, 2021

ericfab179 commented Oct 9, 2021

felipemontoya commented Oct 13, 2021

felipemontoya commented Oct 13, 2021

regisb left a comment

edx-status-bot commented Oct 15, 2021

felipemontoya commented Oct 15, 2021

openedx-webhooks commented Oct 15, 2021

edx-pipeline-bot commented Oct 15, 2021

edx-pipeline-bot commented Oct 15, 2021

	emsg = response_json["emsg"]
	emsg = response_json.get("emsg")


		exception = SafeExecException(emsg)

		globals_dict.update(response_json["globals_dict"])

	globals_dict.update(response_json["globals_dict"])
	globals_dict.update(response_json.get("globals_dict"))


		globals_dict.update(response_json["globals_dict"])

		except RequestException:

Optionally run Codejail in a external service using REST API. #27795

Optionally run Codejail in a external service using REST API. #27795

Conversation

ericfab179 commented Jun 1, 2021

openedx-webhooks commented Jun 1, 2021 • edited Loading

natabene commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdpinch commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfab179 commented Jul 22, 2021

nedbat commented Jul 29, 2021

ormsbee commented Aug 1, 2021

ormsbee commented Aug 1, 2021

ericfab179 commented Aug 6, 2021

ericfab179 commented Aug 6, 2021

pdpinch commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfab179 commented Oct 9, 2021

felipemontoya commented Oct 13, 2021

felipemontoya commented Oct 13, 2021

regisb left a comment

Choose a reason for hiding this comment

edx-status-bot commented Oct 15, 2021

felipemontoya commented Oct 15, 2021

openedx-webhooks commented Oct 15, 2021

edx-pipeline-bot commented Oct 15, 2021

edx-pipeline-bot commented Oct 15, 2021

openedx-webhooks commented Jun 1, 2021 •

edited

Loading