Skip to content
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

python: fix handling of JobInfo properties and to_dict() method with missing attributes #5493

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Oct 10, 2023

This PR fixes #5491, however it does it in two ways

  • fix handling of the t_remaining JobInfo attribute when expiration and/or status attrs are missing
  • skip attributes that raise an exception in to_dict()

This allows to_dict() to work with JobInfo objects returned from flux.job.result().

Copy link
Member

@jameshcorbett jameshcorbett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Problem: When a JobInfo is not fully populated with attributes,
e.g. when not all attrs were requested in a job-list RPC, or
when using flux.job.result(), the to_dict() method raises an
AttributeError exception because some derived attributes are
not present in the JobInfo object.

Skip attributes that raise AttributeError.
Problem: If the status and/or expiration properties of a JobInfo
object are not set, then getting the t_remaining attribute throws
an AttributeError, but this attribute should instead be set to 0.0
when unset.

Add a try/except block around the calculation of remaining time and
return 0.0 for this attribute if AttributeError or KeyError are caught.
Problem: Testing of the returned JobInfo objects from flux.job.result()
does not include ensuring that the to_dict method works and that
accessing the t_remaining property doesn't raise an exception.

Enhance the testing of flux.job.result() to include this testing.
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #5493 (f2a2482) into master (4943070) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5493   +/-   ##
=======================================
  Coverage   83.65%   83.65%           
=======================================
  Files         484      484           
  Lines       81487    81493    +6     
=======================================
+ Hits        68169    68176    +7     
+ Misses      13318    13317    -1     
Files Coverage Δ
src/bindings/python/flux/job/info.py 93.76% <100.00%> (+0.60%) ⬆️

... and 6 files with indirect coverage changes

Copy link
Member

@wihobbs wihobbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much Mark!

@wihobbs
Copy link
Member

wihobbs commented Oct 10, 2023

I tested it in a container with the same commands I put in #5491 and it works great.

@grondo
Copy link
Contributor Author

grondo commented Oct 10, 2023

Thanks @jameshcorbett and @wihobbs. I'll set MWP.

@mergify mergify bot merged commit e79a7e2 into flux-framework:master Oct 10, 2023
32 checks passed
@grondo grondo deleted the fix-to_dict branch October 10, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python: some JobInfo attributes don't port to_dict()
3 participants