Python 3.x fixes and codejail config #58
Conversation
|
Thanks for the pull request, @shaidar! I've created OSPR-5693 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:
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. |
openedx-webhooks
added
needs triage
open-source-contribution
|
@shaidar Thank you for your contribution. I am lining this up for our review. |
openedx-webhooks
added
awaiting prioritization
and removed
waiting on author
|
Not sure, but I think this might have been a regression related to #56 @UsamaSadiq or @iamsobanjaved can you take a look? As far as I can tell, this code isn't covered by tests. We were going to hold off until we could get some tests written, but I just heard this is causing problems for a final exam that starts today. |
|
Thanks, I did mean #57. And I'm probably wrong about the cause the regression. It may be more of an environment issue -- I don't see how the code we're patching here could have ever worked, except that it wasn't touched in a Python 2.7 |
|
I think it would be better to merge the PR rather than https://github.com/edx/xqueue-watcher/pull/59/files, but since there is an exam going on until 3/30 and edx has rolled back to the version of the xqwatcher from 3/8 in prod I think it may be better to leave the service on python 2.7 until Monday. Note that this code likely won't get pulled in until we unpause our xqwatcher pipelines and do a deploy on python 3.5: https://github.com/mitodl/graders-mit-600x/blob/master/graders/python3graders |
|
@shaidar Thanks for this effort. Just a heads-up, before merging this PR, I'll squash the commits and rewrite the commit message so it follows the Conventional Commit guidelines. |
| @@ -238,11 +239,7 @@ def _tokens(code): | |||
| A wrapper around tokenize.generate_tokens. | |||
| """ | |||
| # Protect against pathological inputs: http://bugs.python.org/issue16152 | |||
There was a problem hiding this comment.
This comment should also be removed, as the bug is fixed and the bug workaround is removed.
|
@shaidar or @blarghmatey, we are planning to ship this this morning. Would one of you be able to update the PR to resolve the comment by @doctoryes? |
Fixes incompatible tokenization of user submitted code when running under Python 3.x. This is used to add checks to ensure that students are using specific keywords or language features.
Fix bug when using codejail limits in xqueue-watcher
When the `limits` object is set in the codejail configuration block it causes the command used for launching a watcher process to be invalid. This is because there was a variable name used in a for loop that shadowed a variable from an outer scope that is then returned from the `enable_codejail` method that is used as an argument for the execution.
Fixed invalid conversion of conditional dictionary iterator
The PR that removed the usage of `six` failed to wrap the conditional `environment or {}` in parentheses before calling `.items()` on the resulting value. This would lead to trying to iterate over _either_ `environment` or `{}.items()` which is not the correct behavior.
|
@shaidar 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This PR addresses the following two issues:
importstatements weren't being read and causing an error to be thrown that no import statement are specified.limitsobject is set in the codejail configuration block it causes the command used for launching a watcher process to be invalid. This is because there was a variable name used in a for loop that shadowed a variable from an outer scope that is then returned from theenable_codejailmethod that is used as an argument for the execution.sixfailed to wrap the conditionalenvironment or {}in parentheses before calling.items()on the resulting value. This would lead to trying to iterate over eitherenvironmentor{}.items()which is not the correct behavior.