-
Notifications
You must be signed in to change notification settings - Fork 4
Codejail REST API service Flask - MVP #2
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
Conversation
Run configure codejail method before importing safe_exec modules. Adjust settings and wsgi files.
codejailservice/config.py
Outdated
| 'user': 'sandbox', | ||
|
|
||
| # Configurable limits. | ||
| # Setting all of them to 0 to disable limits in conatiners. |
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.
Typo: containers
codejailservice/app.py
Outdated
| from copy import deepcopy | ||
| from flask import Flask, Response, jsonify, request | ||
|
|
||
| LOG = logging.getLogger(__name__) |
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.
The LOG is not used, I imagine you imported it just in case
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.
If we want to use logging in Flask we need to add the right configuration to get it working right?
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.
@jfavellar90 Sure, we need to configure it, do you think we should do that in this MVP ?
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 think so, at least a raw version that can be improved afterward.
| @@ -0,0 +1,8 @@ | |||
| import os | |||
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.
Is this running as a single process in the container? is it possible to use Gunicorn or awsgi? Or is it outside of this MVP?
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.
In development it runs a single process in the container https://github.com/eduNEXT/tutor-contrib-codejail/blob/b8857457f25f5481a5fdc56d7742f5f6b2420fb9/tutorcodejail/patches/local-docker-compose-dev-services#L2 but in local and k8s it should run using gunicorn
https://github.com/eduNEXT/tutor-contrib-codejail/blob/b8857457f25f5481a5fdc56d7742f5f6b2420fb9/tutorcodejail/templates/codejail/build/codejail/Dockerfile#L79
Are you agree with that @jfavellar90 ?
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've no problem with Gunicorn but I'm wondering if we should use uwsgi to be consequent with the other tutor services (for instance edxapp )
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.
It was updated to use uwsgi in local and k8s.
711711e to
cd90a1a
Compare
Fix empty python path case. Basic configuration for logging.
cd90a1a to
29d0b18
Compare
felipemontoya
left a comment
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 tested this today using an "august" version of edx-platform with the changes of https://github.com/edx/edx-platform/pull/27795 and it worked as a POC.
I think we should merge this as is and iterate on improving the QA and documentation and fixes that come along as we find them.
Having this in the PR is making it more complicated for people just looking at this now.
No description provided.