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
Executor rewrite and new features #144
Conversation
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.
Generally speaking, this is too much in one pull request. It is at least two of them, one for the new JSON-based environment check configuration, and one for the locking modifications. There would normally be even a third one for the additional server variable being introduced for logging purposes.
Anyway.
The biggest blocker for me is the backward-incompatible change of the configuration file (handling). I trust you with the locking code and assume that you performed some tests on Unix, too.
conf = _infos_cmd("cc -v") | ||
try: #read keys and values from JSON file | ||
import simplejson | ||
with open('environment.json','rb') as f: |
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.
Where do you expect environment.json
to live in the file system? What is the purpose of having this code part, and the reading from the configured JSON file afterwards?
# Load python directory of additional environment information to query from JSON file for | ||
# testmachine registration. Only one-dimensional directories are supported! For syntax see | ||
# simplejson package documentation. | ||
# { "Make" : "make -v", |
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 would expect to have all current information gathering calls (Scala, OpenCL, ...) also in the new version of the template.
Another problem is that your changes break all existing executor installations, since this configuration element does not exist there. I would propose that you perform an according check of the existing config file as part of the "opensubmit-exec configure" management command. Printing an according warning should be enough.
@@ -10,9 +10,9 @@ | |||
import time | |||
from threading import Timer | |||
from datetime import datetime, timedelta | |||
from lockfile import FileLock |
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.
What is the problem with the lockfile package? It seems to be the right choice for having cross-platform locking. Some explanation for the "manual" solution would be good.
@@ -85,17 +85,6 @@ def console_script(): | |||
from . import send_config | |||
send_config(EXECUTOR_CONFIG_FILE) | |||
exit(0) | |||
if "unlock" in sys.argv: |
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.
Any reason that this is no longer needed?
@@ -178,6 +178,7 @@ def jobs(request): | |||
response = HttpResponse(f, content_type='application/binary') | |||
response['Content-Disposition'] = 'attachment; filename="%s"' % sub.file_upload.basename() | |||
response['SubmissionFileId'] = str(sub.file_upload.pk) | |||
response['SubmissionId'] = str(sub.pk) |
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.
Good idea, I like it.
Time moves on. The upcoming 0.7.0 release moves OpenSubmit to Python 3.6 only. The executor was completely re-designed as part of this, which makes your pull request more or less invalid. The rewrite is the starting point to work on all of these issues: https://github.com/troeger/opensubmit/milestone/19 Please note that the locking code is already changed. This might at least solve your Windows problems, but I cannot promise that. We could still try to take your different changes as proposals for solving the issues listed above. I will check that. |
I have rewritten the locking because of stability of the windows port. I have changed the executor registration code. First attempt was to includes additional environment values in the registration process. Therefor I added a executor value (custom_variables) for reading additional key values pairs from a JSON file.
Then I changed the other values to JSON file also. 2 values are hard coded in the executor because on special handling (opencl, cc).