Implement promised requirements (resolves #729) #791
Conversation
hannes-ucsc
changed the title
|
Commit message is wrong. |
| :type memory: int or string convertable by bd2k.util.humanize.human2bytes to an int | ||
| """ | ||
| self.cores = cores | ||
| :type cores: int; Can also accept a PromisedRequirement object |
There was a problem hiding this comment.
Read PEP 0484 for a specification . The PyCharm support web site has a page about type hinting, too.
|
You need to modify setup.py and mention dill as a requirement. |
| @@ -254,6 +298,32 @@ def encapsulate(self): | |||
| """ | |||
| return EncapsulatedJob(self) | |||
|
|
|||
|
|
|||
| @staticmethod | |||
| def update_kwargs(kwargs): | |||
There was a problem hiding this comment.
If you modify kwargs in place you shouldn't return it. Returning it would indicate to the caller that you return a copy and that you didn't touch the argument, but the copy.
There was a problem hiding this comment.
The name of this method is too unspecific.
| @@ -1654,3 +1757,29 @@ def promisedJobReturnValueUnpickleFunction(jobStoreString, jobStoreFileID): | |||
| copy_reg.pickle(PromisedJobReturnValue, | |||
| promisedJobReturnValuePickleFunction, | |||
| promisedJobReturnValueUnpickleFunction) | |||
|
|
|||
|
|
|||
| class PromisedRequirement(object): | |||
There was a problem hiding this comment.
Document the class, including its use as a callable.
|
Jenkins, test this please. |
|
|
||
| def increment_counter(filepath): | ||
| """ | ||
| Without the second argument, increment counter, sleep one second and decrement. |
There was a problem hiding this comment.
What second argument? This comment is redundant. The code below says the same thing, and is actually more accurate since here you say that it waits for 1s which isn't true.
|
Alright, so the main things are: I don't like how you handle promise_disk etc, i.e. the shadow attributes. Ideally, I would want to get rid of them. I'm worried that resetting the requirements to the default values will have unintended consequences. The single machine batch system implements a strict FIFO regime and looks at the requirements very late. The Mesos batch system puts jobs into separate queues, based on their requirements, so it looks at them earlier. Now that I am saying it, your tests should be executed against every batch system. You can either leave your tests in a separate module and replicate the structure of the existing batch system tests. Or you add your tests to the existing batch system tests. I think I'd prefer the former. |
|
We might not be able to support static DAGs. And we might have to defer the submission of a job with promised requirements to until after the promises have been resolved. |
|
And keep this branch rebased on master, once per day. |
|
@jpfeil, PR #823 is under review by @benedictpaten, so we might not get to merging it today. It failed for unrelated reasons but passes your test and all other tests that could be affected by promises. You may just want to rebase this branch onto the branch for #823 avoid being blocked that way. Also, if I were you, I would ditch your personal fork and start pushing directly to the upstream. The fact that the branch for #818 was in your fork—to which I can't push—made it necessary for me to close #818 and start #823 in its place. |
|
Continued in #828 |
Resolves #729