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

add ResourceJournalConsumer and EventLogFormatter classes and use them to standardize flux resource eventlog #6614

Merged
merged 13 commits into from
Feb 8, 2025

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Feb 7, 2025

The main purpose of this PR is to add the Python ResourceJournalConsumer class, which mimics the existing job manager JournalConsumer class. In fact, most of the original JournalConsumer class implementation is moved to an abstract JournalConsumerBase class, then both the job manager JournalConsumer and ResourceJournalConsumer classes inherit from that.

In addition, the flux resource eventlog utility is modified to have similar options to the rest of the eventlog utilities in Flux (--format, --time-format, --color, --human)using a new EventLogFormatter class and the newly added ResourceJournalConsumer class.

@grondo grondo force-pushed the resource-journal-consumer branch 2 times, most recently from c2f2388 to cbb5beb Compare February 8, 2025 01:11
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

Nicely done! Good docs too!

I gave this an admittedly light skim and tried it out a bit and LGTM! Very nice to have the flux resource eventlog command dressed up like one expects.

subclasses will follow the interface documented here.

A journal consumer is created by passing the constructor an open
:obj:`~flux.Flux` handle, the topic string of the journal servicealong
Copy link
Member

Choose a reason for hiding this comment

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

servicealong needs a space

@grondo grondo force-pushed the resource-journal-consumer branch from cbb5beb to 10502d9 Compare February 8, 2025 04:22
grondo added 13 commits February 8, 2025 06:24
Problem: The EventLogEvent class is defined in flux.job.event, but
the eventlog format is not specific to jobs.

Move the EventLogEvent class up a level to eventlog.py. For backwards
compatibility, retain the ability to import EventLogEvent from flux.job
and flux.job.event.
Problem: The JournalConsumer class in flux.job is specific to the
job manager journal, but there may be other services that
implement a journal-like interface.

Add an abstract base class for journal consumer interfaces in
flux.abc.JournalConsumerBase. Include a JournalEventBase class that
derives from EventLogEvent.

Most of the journal consumer interface is implemented in this class,
with the expectation that derived classes only need to implement the
few abstract methods left open for implementations.
Problem: The flux.job.JournalConsumer class duplicates code from
the JournalConsumerBase class.

Make flux.job.JournalConsumer a subclass of the JournalConsumerBase
abstract base class.
Problem: There is no interface for the resource module journal.

Add flux.resource.journal.JournalConsumer as a subclass of the
JournalConsumerBase abstract base class.
Problem: There are no test of the Python ResourceJournalConsumer
class.

Add a Python based test in t/python/t0032-resource-journal.py.
Problem: Python utilities do not have access to the eventlog formatter
class used by all C utilities for standard eventlog presentation.

Add a Python EventLogFormatter class that mimics the C version.
Problem: There are no tests of the Python EventLogFormatter class.

Add a new test t/python/t0033-eventlog-formatter.py.
Problem: `flux resource eventlog` doesn't have the same options and
functionality as other eventlog commands.

Make the options and functionality of `flux resource eventlog`
match the standard Flux utility eventlog interface using the Python
EventLogFormatter class.
Problem: t2355-resource-journal.t assumes the output of `flux resource
eventlog` is JSON, but this is no longer true.

Pass `-f json` to the command to force JSON output.
Problem: Newly added options to `flux resource eventlog` are not
tested in t2355-resource-journal.t.

Expand testing to cover new options.
Problem: The `flux resource eventlog` implementation uses the
resource.journal RPC directly, but there's now a Python class
for that.

Use ResourceJournalConsumer to simplify the `flux resource eventlog`
implementation.
Problem: New options to `flux resource eventlog` are not documented
in flux-resource(1).

Update the docs.
Problem: The `eventlog` subcommand of flux-resource(1) does not have
tab completions.

Add them to etc/completions/flux.pre.
@grondo grondo force-pushed the resource-journal-consumer branch from 10502d9 to 90ef568 Compare February 8, 2025 14:24
@grondo
Copy link
Contributor Author

grondo commented Feb 8, 2025

Thanks! I've rebased and fixed that typo and will set MWP now.

@mergify mergify bot merged commit 779a41d into flux-framework:master Feb 8, 2025
35 checks passed
Copy link

codecov bot commented Feb 8, 2025

Codecov Report

Attention: Patch coverage is 96.81979% with 9 lines in your changes missing coverage. Please review.

Project coverage is 83.86%. Comparing base (c205af7) to head (90ef568).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/bindings/python/flux/abc/journal.py 96.58% 4 Missing ⚠️
src/bindings/python/flux/eventlog.py 97.34% 3 Missing ⚠️
src/bindings/python/flux/resource/journal.py 93.75% 1 Missing ⚠️
src/cmd/flux-resource.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6614      +/-   ##
==========================================
+ Coverage   83.81%   83.86%   +0.05%     
==========================================
  Files         530      534       +4     
  Lines       88237    88381     +144     
==========================================
+ Hits        73955    74120     +165     
+ Misses      14282    14261      -21     
Files with missing lines Coverage Δ
src/bindings/python/flux/abc/__init__.py 100.00% <100.00%> (ø)
src/bindings/python/flux/job/event.py 100.00% <100.00%> (+1.53%) ⬆️
src/bindings/python/flux/job/journal.py 100.00% <100.00%> (+0.90%) ⬆️
src/bindings/python/flux/resource/__init__.py 100.00% <100.00%> (ø)
src/bindings/python/flux/resource/journal.py 93.75% <93.75%> (ø)
src/cmd/flux-resource.py 95.10% <95.00%> (+0.83%) ⬆️
src/bindings/python/flux/eventlog.py 97.34% <97.34%> (ø)
src/bindings/python/flux/abc/journal.py 96.58% <96.58%> (ø)

... and 11 files with indirect coverage changes

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.

2 participants