job-info: support lookup of updated jobspec, remove manual construction of updated R / jobspec #5635
Conversation
|
Reminder: we created https://flux-framework.readthedocs.io/projects/flux-rfc/en/latest/spec_41.html as a basis for discussing this one. I haven't looked at what you proposed yet but just wanted to let you know that's out there. |
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
3 times, most recently
from
d65bc7e to
b2c26ce
Compare
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
2 times, most recently
from
abf4b17 to
7dedabb
Compare
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
4 times, most recently
from
068220f to
c9f3467
Compare
Yup! started PR flux-framework/rfc#407 to discuss this PR's pre-req PR (#5633) ... although now I see that I left that RFC PR as WIP, so maybe I should make that non-WIP. |
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
from
c9f3467 to
7900640
Compare
|
re-pushed, rebasing on #5633 .. only change amongst this PR's commits was that fixed a comment that needed to say "jobspec & R" vs "R" |
Problem: In one python test, data is retrieved after an RPC call but the data is never verified. Call the helper function to verify the retrieved data.
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
from
7900640 to
f33f6ec
Compare
| @@ -37,7 +37,7 @@ enum job_lookup_flags { | |||
| FLUX_JOB_LOOKUP_JSON_DECODE = 1, | |||
| /* get current value of special fields by applying eventlog | |||
| * updates for those fields | |||
| * - currently works for R | |||
| * - currently works for jobspec and R | |||
There was a problem hiding this comment.
commit message typo: "an current/update jobspec"
Also, perhaps capitalize the first "current" because it reads as "the current flag" instead of "the flag that is named current" and is a bit confusing with all the other currents in the description.
Problem: It is inconvenient that lookup w/ the CURRENT flag and update-watch support reading/watching a current/updated R but not a current/updated jobspec. Support returning a current/updated jobspec.
Problem: There is no coverage for job-info lookups w/ the current flag or update-watch for the jobspec key. Add some coverage in t2233-job-info-update.t.
Problem: flux job info manually reconstructs a current jobspec from the jobspec and eventlog. However the job-info.lookup service w/ the CURRENT flag could be used instead. Update code to use the lookup service w/ current flag instead of manually reconstructing the current jobspec.
Problem: In the kvslookup.py module, the current jobspecs and R values are done manually. They could instead take advantage of the job-info.lookup service and the recently added CURRENT flag. Update kvslookup.py to use job-info.lookup with the CURRENT flag. Remove code that manually constructs current jobspecs and R.
chu11
force-pushed
the
job_info_update_jobspec_remove_manual
branch
from
f33f6ec to
d5b38ff
Compare
|
@grondo thanks! re-pushed with that commit message cleanup & setting MWP |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5635 +/- ##
==========================================
- Coverage 83.48% 83.48% -0.01%
==========================================
Files 488 488
Lines 83421 83366 -55
==========================================
- Hits 69643 69596 -47
+ Misses 13778 13770 -8
|
Problem: It is inconvenient that lookup w/ updates and update-watch support reading/watching an updated R but not an updated jobspec.
Support returning an updated jobspec.
Then remove the manually updated jobspec/R code in
flux-joband the Pythonkvslookuplibrary.Built on top of #5633.
Could potentially be split into two PRs ....