search

flux-framework / flux-core

core services for the Flux resource management framework
GNU Lesser General Public License v3.0
168 stars 50 forks source link

python: add `flux.job.JournalConsumer` class with a simplified interface for job manager journal consumers #6386

Closed grondo closed 1 month ago

grondo commented 1 month ago

This PR adds the JournalConsumer class described in #4569 with the addition of some tests.

Putting up here now for review so we can get some official feedback on the interface.

garlick commented 1 month ago

Took me a little while to find the auto-generated docs in our python docs. Had to use the search box but here it is:

https://flux-framework--6386.org.readthedocs.build/projects/flux-core/en/6386/python/autogenerated/flux.job.journal.html#module-flux.job.journal

I did notice "Syncrhonously" was misspelled in the poll() docs. Hmm, I thought our typo checker would have caught that.

Feel free to ignore, but for extant use cases in the source tree, time sorting of historical events wasn't required and the sentinel demarcating history from current events was useful. I wonder if making history sorting optional or maybe adding an option to to signal the demarcation with an empty JournalEvent might be useful down the line? Meh, would be easy to add later I'm sure.

grondo commented 1 month ago

I was actually going to ask if we thought the sentinel would be useful to get, but I wasn't quite sure how to present it.

I guess one thought is that if a use case doesn't require the events ordered, then probably this class is not necessary and they can use the RPC directly?

grondo commented 1 month ago

(and FYI I found some issues with handling of ENODATA so I'm reworking some of the code now.)

garlick commented 1 month ago

Great point!

grondo commented 1 month ago

I've repushed here with support for the sentinel event if include_sentinel is True in the JournalConsumer initializer. (The default is False). The sentinel event is represented by an empty event defined by a flux.job.journal.SENTINEL_EVENT constant. Tests have been added for include_sentinel=True.

I also expanded the docs, trying to use sphinx references where possible. Note that I've dropped the pre-commit check-docstring-first check, which I don't think we really need in Flux and having it effectively makes it impossible to document module variables and constants. (it threw an error with the SENTINEL_EVENT documentation here)

grondo commented 1 month ago

It would probably be good if @wihobbs gave this iteration of JournalConsumer a try, and also if @morrone gave an ACK. (generated docs from this PR are linked above by @garlick)

garlick commented 1 month ago

The docs look much nicer!

grondo commented 1 month ago

Maybe it would be better to point directly to RFC 21? Maybe even to the "Event Descriptions" section?

Good idea!

grondo commented 1 month ago

I pushed a new version of this PR which adds references to RFCs where appropriate as suggested above. (I've added an rfc domainref to doc/conf.py to make it easier to reference RFCs in the docs here)

Also expanded the description of the JournalConsumer class a tiny bit.

Edit: Noticed a bug in the __str__ method of JournalEvent and missing tests, so pushed again.

grondo commented 1 month ago

Thanks for the approval @wihobbs! I'll set MWP then :-)

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 99.03846% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.51%. Comparing base (53c4acd) to head (826cb29). Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/bindings/python/flux/job/journal.py 99.02% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6386 +/- ## ========================================== + Coverage 83.49% 83.51% +0.02% ========================================== Files 524 525 +1 Lines 87721 87825 +104 ========================================== + Hits 73239 73348 +109 + Misses 14482 14477 -5 ``` | [Files with missing lines](https://app.codecov.io/gh/flux-framework/flux-core/pull/6386?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework) | Coverage Δ | | |---|---|---| | [src/bindings/python/flux/job/\_\_init\_\_.py](https://app.codecov.io/gh/flux-framework/flux-core/pull/6386?src=pr&el=tree&filepath=src%2Fbindings%2Fpython%2Fflux%2Fjob%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL2JpbmRpbmdzL3B5dGhvbi9mbHV4L2pvYi9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | | | [src/bindings/python/flux/job/journal.py](https://app.codecov.io/gh/flux-framework/flux-core/pull/6386?src=pr&el=tree&filepath=src%2Fbindings%2Fpython%2Fflux%2Fjob%2Fjournal.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework#diff-c3JjL2JpbmRpbmdzL3B5dGhvbi9mbHV4L2pvYi9qb3VybmFsLnB5) | `99.02% <99.02%> (ø)` | | ... and [15 files with indirect coverage changes](https://app.codecov.io/gh/flux-framework/flux-core/pull/6386/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework)