Conversation
jonasbardino
left a comment
jonasbardino
left a comment
There was a problem hiding this comment.
Approved with a couple of comments to look into
|
I think the |
| new_keys.append(k) | ||
|
|
||
| lookupdict = dict(list(lookupdict.items()) + list(sumdict.items())) | ||
| keys = list(set(new_keys)) |
There was a problem hiding this comment.
I think this removal is too greedy. AFAICT it should not remove the entire for-loop and in particular the else case, but only the if part and the sums+concat lines.
There was a problem hiding this comment.
You are absolutely right, good catch
There was a problem hiding this comment.
Looking at the code once more I'll say that the whole section should be eliminated as suggested in the commit
There was a problem hiding this comment.
Without being an expert in this code it appears to handle any kind of resource in the 'machine' conditional and specifically general (non-sandbox) resources in the else case, so I don't see why that part should go. It looks like such non-sandbox resource IDs are added to keys after the loop and used in the for k in keys loop later.
That said, I doubt we have this backend in action anywhere. The whole couchdb stats work was grid.dk AFAIK.
There was a problem hiding this comment.
Did you look at the complete code?
As far as I can tell this section sums up sandbox resources and add non-sandbox resources unconditionally to the new_keys that in the end is assigned to keys:
keys = list(set(new_keys))
All resources are assigned to keys prior to this section, that is if we drop this section all resources will be treated as non-sandboxed resources.
| self.logger.info('backfill remaining: %(CPUCOUNT)s %(NODECOUNT)s' | ||
| % remaining) | ||
| next_job = self.schedule(remaining, must_match={ | ||
| next_job = self.schedule(remaining, must_match={ # pylint: disable=assignment-from-none |
There was a problem hiding this comment.
I don't like pylint disables unless strictly needed, and would much prefer that we fix the potential None issue it complains about.
It's perfectly okay to make a simple placeholder 'Fix assign from None lint warning in scheduler' PR, which just highlights the issue and then point to that as proof that it is nothing new. Then we can always fix the issue later.
The easiest is to just add a line to the offending file in the PR branch.
There was a problem hiding this comment.
So we should just accept that the linter fails at this one and create a PR for it ?
There was a problem hiding this comment.
Yes, I'd do like in #338 and point to a similar new PR here.
There was a problem hiding this comment.
Ok, I can't wrap my head around how you then verify that this PR is correct linting wise, but will remove # pylint: disable=assignment-from-none again when ready for merge.
There was a problem hiding this comment.
Link to the placeholder PR triggering the exact same lint warnings here and write a comment here confirming that this job only reports those lint errors in the lint job output. Done.
jonasbardino
left a comment
jonasbardino
left a comment
There was a problem hiding this comment.
There are a few new issues to fix before merging. Please refer to my inline comments.
jonasbardino
left a comment
jonasbardino
left a comment
There was a problem hiding this comment.
No, I did not actually suggest reintroducing java as a job handling language (genjobscriptjava) as that was specific to wrap and deliver jobs to oneclick resources AFAIK.
I just requested that the simple N-queens example executing a pre-compiled java executable (jar) under any job handler (probably bash) using whatever java env available at the given resource does not get thrown out in the process. Just like I wouldn't throw out python example jobs in case we decided to retire the python job handler (genjobscriptpython).
Right, I misread the example. |
2 participants
Retire sandbox and SSS resources