Support job name #2249
Support job name #2249
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #2249 +/- ##
==========================================
- Coverage 31.67% 31.37% -0.31%
==========================================
Files 74 81 +7
Lines 7308 8640 +1332
==========================================
+ Hits 2315 2711 +396
- Misses 4993 5929 +936
Continue to review full report at Codecov.
|
| if p['uid'] == pid: | ||
| # we do not force unique job_names and multiple pilots may have | ||
| # the same job_name. By checking if p['uid'] is in PMGR pilots | ||
| # we ensure that each pilot is checked only once. |
There was a problem hiding this comment.
I wonder if we should actually require unique names. I mean, otherwise a cancel() call can well end up on the wrong pilot if we did not get the order right in this piece of code?
There was a problem hiding this comment.
I checked with multiple pilots as well. It requires the name only during starting the pilots. After that it uses the job id that the scheduler provides to identify different jobs.
There was a problem hiding this comment.
Yes, sure - but I am not sure the mapping from pilot UID to pilot job is unique then. For uniform pilots is should not matter, and we don't regularly run non-uniform pilots in the same session, so this is really an edge case. And I don't have suggestion... :-P So, ultimately this looks good to me - thanks!
There was a problem hiding this comment.
BTW just to confirm, isn't the order of jobs, jobs descriptions and pilots is consistent? (and kinda it should be forced if not). Thus the comparison of pilot and job names shouldn't be matter (and that extra loops could be skipped)
for j, jd, pilot in zip(jc.get_tasks(), jd_list, pilots):
...There was a problem hiding this comment.
@mtitov I am not sure I understand. Can you expand it a bit?
There was a problem hiding this comment.
@iparask yeah, that my proposal will allow to remove L835-853: list jd_list is formed from list pilots (link), thus jobs descriptions order corresponds to the order of pilots, and we don't need to look for which pilot is related to which job description based on their names/uids, but rather use same index in their lists. And this line could be converted into for j, jd, pilot in zip(jc.get_tasks(), jd_list, pilots). Hope I didn't bring more confusion
There was a problem hiding this comment.
I see. This makes the assumption that the pilots are in the same order as their corresponding SAGA jobs. @andre-merzky is this true? If it is I think implementing @mtitov's suggestion makes sense
There was a problem hiding this comment.
we didn't resolve this? btw, there already was an assumption about SAGA jobs and job descriptions
# we assume here that the tasks arrive in the same order as the job
# descriptions. For uniform sets of pilots the order does not matter
# much though. Either way, this needs confirming on SAGA level
# FIXME
and list jd_list was created from list pilots, which assumes that the order is preserved. Just not dealing with pilot/job names (at least here) simplifies this part.
This PR makes sure that the pilot manager gets the correct information when launching pilots that have the
job_nameattribute set. It also takes into account that multiple pilots have the samejob_name.It closes #2248.