`formatter`: add new `JobsFormatter` class, restructure `view-job-records` to use new class #563

cmoussa1 · 2025-01-24T01:21:15Z

Problem

The formatter module in flux-accounting does not have a class for formatting the output of job records which would be useful because the flux-accounting Python bindings manually define the formatting of these job records within its bindings.

This PR creates a new class called JobsFormatter, a subclass of flux.util.OutputFormat, which can take a Python format string to allow a user to customize their output, similar to flux-jobs:

flux account view-job-records -o "{userid:<8} | {nnodes:<6} | {bank:<8}" 
userid   | nnodes | bank    
5001     | 1      | bankB   
5002     | 1      | bankB   
5002     | 1      | bankB   
5001     | 1      | bankA   
5001     | 1      | bankA

view-job-records is restructured to use the new JobsFormatter class instead of defining its own print function as well as modified to include a -o, --format optional argument to the command to allow the user to specify a format string to customize their output.

Finally, a couple of the view-job-records tests are cleaned up to account for the use of the new formatter class.

jameshcorbett

This looks good but I wonder if for consistency with flux jobs the subcommand might instead take an option like (taken from flux jobs)

 -o, --format=FORMAT        Specify output format using Python's string format syntax or a defined format by name (use 'help' to get a list of names)

example usage:

[corbett8@rzadams1:~]$ flux jobs -A -o "{id} |  {username}"
JOBID |  USER
f2DVdVNa66iB | user1
f2DbuK95EHuq |  user2
[corbett8@rzadams1:~]$ flux jobs -A -o "{id} |  {username} {foo}"
flux-jobs: ERROR: Error in user format: Unknown format field: foo

More info here: https://flux-framework.readthedocs.io/projects/flux-core/en/latest/man1/flux-jobs.html#output-format

jameshcorbett · 2025-02-03T21:09:08Z

src/cmd/flux-account.py

+        type=str,
+        help="list of fields to include in output",
+        default="userid,username,jobid,t_submit,t_run,t_inactive,nnodes,project,bank",
+        metavar="USERID,USERNAME,JOBID,T_SUBMIT,T_RUN,T_INACTIVE,NNODES,PROJECT,BANK",


Does having this long metavar not make the help/usage message kind of weird looking?

I don't think so? But maybe it's just my terminal?

usage: flux-account.py view-job-records [-h] [-u USERNAME] [-j JOBID] [-a START TIME] [-b END TIME] [--project PROJECT] [--bank BANK] [--fields USERID,USERNAME,JOBID,T_SUBMIT,T_RUN,T_INACTIVE,NNODES,PROJECT,BANK]

I could probably just break this into a multi-line string though in flux-account.py. FWIW, it's less than 90 characters which I think is compatible with black formatting.

jameshcorbett · 2025-02-03T21:11:42Z

src/bindings/python/fluxacct/accounting/jobs_table_subcommands.py

+        "bank",
+    ]
+    fields = fields or default_fields
+    invalid_fields = [field for field in fields if field not in default_fields]


If I'm reading this right, the user can suppress some fields if they choose but there aren't any hidden fields they can see by changing the format?

Yes. The user can choose to only include a couple of the fields listed, but can only choose from those fields.

cmoussa1 · 2025-02-04T04:26:19Z

Thanks for reviewing this @jameshcorbett! I think that'd be a good improvement to this. Let me think about how I might be able to take a format string. It might take some extra thought because I believe flux jobs just prints stuff but these subcommands need to return strings in the function... I'll do some digging!

cmoussa1 · 2025-02-04T22:29:10Z

@jameshcorbett I went ahead and restructured this PR a little bit to have the JobsFormatter class actually be a subclass of flux.util.OutputFormat, which allows it to take a format string (like flux jobs). You'll notice that I had to create a mapping for the headings (I referenced this class in flux-core), so I just kept the headings the same across the board.

jameshcorbett

Looks amazing, two little comments

jameshcorbett · 2025-02-05T18:35:31Z

src/bindings/python/fluxacct/accounting/formatter.py

+        # get the current class for creating a new formatter instance:
+        cls = self.__class__
+        # create new instance of the current class from filtered format:
+        formatter = cls(newfmt, headings=self.headings, prepend=self.prepend)


I noticed this comes from flux-core but I think I would prefer type(self) instead of self.__class__, maybe type(self)(newfmt, headings=...

jameshcorbett · 2025-02-05T18:42:46Z

src/bindings/python/fluxacct/accounting/formatter.py

+        for _, field, _, _ in format_list:
+            if field and field in self.headings:
+                self.headings[field] = field
+        super().__init__(fmt)


Should prepend be passed on to the superclass' __init__?

Problem: The formatter module in flux-accounting does not have a class for formatting the output of job records which would be useful because the flux-accounting Python bindings manually define the formatting of these job records within its bindings. Create a new class called JobsFormatter, a subclass of flux.util.OutputFormat, which can take a Python format string to allow a user to customize their output, similar to flux-jobs.

Problem: The convert_to_str() function defines its own formatting for job records to be returned with the view-job-records command, but the formatter module in flux-accounting now has its own JobsFormatter class that does this (and also includes custom formatting). Edit the convert_to_str() function to create a JobsFormatter object, initialized with a Python format string. Add a "format" optional argument to the view-job-records command to allow the user to specify a custom format string for their output.

Problem: The output of the view-job-records command has been slightly adjusted as a result of including custom formatting in its output, but there are a couple of tests throughout the testsuite that are now out-of-date because they look for the old format. Adjust the tests that look for view-job-records output throughout the testsuite to account for the new formatting. Get rid of a expected file in the testsuite in favor of just creating that file within the test itself.

cmoussa1 · 2025-02-05T22:53:54Z

Thanks for reviewing @jameshcorbett! After an offline discussion it seems like the prepend attribute is probably not needed by this subclass, so I just removed it from __init__() altogether. Will set MWP here

cmoussa1 added improvement Upgrades to an already existing feature commands related to the flux-accounting commands/bindings labels Jan 24, 2025

cmoussa1 force-pushed the customize.view-job-records branch from e1097ff to afb3b61 Compare January 27, 2025 22:45

cmoussa1 requested a review from jameshcorbett February 3, 2025 20:50

cmoussa1 force-pushed the customize.view-job-records branch from afb3b61 to 5e065e4 Compare February 3, 2025 21:08

jameshcorbett reviewed Feb 3, 2025

View reviewed changes

cmoussa1 force-pushed the customize.view-job-records branch from 5e065e4 to 4e6212b Compare February 4, 2025 22:16

jameshcorbett approved these changes Feb 5, 2025

View reviewed changes

cmoussa1 added 3 commits February 5, 2025 14:52

cmoussa1 force-pushed the customize.view-job-records branch from 4e6212b to 36c6549 Compare February 5, 2025 22:53

cmoussa1 added the merge-when-passing label Feb 5, 2025

mergify bot merged commit f951766 into flux-framework:master Feb 5, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`formatter`: add new `JobsFormatter` class, restructure `view-job-records` to use new class #563

`formatter`: add new `JobsFormatter` class, restructure `view-job-records` to use new class #563

cmoussa1 commented Jan 24, 2025 •

edited

Loading

jameshcorbett left a comment •

edited

Loading

jameshcorbett Feb 3, 2025

cmoussa1 Feb 4, 2025

jameshcorbett Feb 3, 2025

cmoussa1 Feb 4, 2025

cmoussa1 commented Feb 4, 2025

cmoussa1 commented Feb 4, 2025

jameshcorbett left a comment

jameshcorbett Feb 5, 2025

jameshcorbett Feb 5, 2025 •

edited

Loading

cmoussa1 commented Feb 5, 2025 •

edited

Loading

formatter: add new JobsFormatter class, restructure view-job-records to use new class #563

formatter: add new JobsFormatter class, restructure view-job-records to use new class #563

Conversation

cmoussa1 commented Jan 24, 2025 • edited Loading

Problem

jameshcorbett left a comment • edited Loading

Choose a reason for hiding this comment

jameshcorbett Feb 3, 2025

Choose a reason for hiding this comment

cmoussa1 Feb 4, 2025

Choose a reason for hiding this comment

jameshcorbett Feb 3, 2025

Choose a reason for hiding this comment

cmoussa1 Feb 4, 2025

Choose a reason for hiding this comment

cmoussa1 commented Feb 4, 2025

cmoussa1 commented Feb 4, 2025

jameshcorbett left a comment

Choose a reason for hiding this comment

jameshcorbett Feb 5, 2025

Choose a reason for hiding this comment

jameshcorbett Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

cmoussa1 commented Feb 5, 2025 • edited Loading

`formatter`: add new `JobsFormatter` class, restructure `view-job-records` to use new class #563

`formatter`: add new `JobsFormatter` class, restructure `view-job-records` to use new class #563

cmoussa1 commented Jan 24, 2025 •

edited

Loading

jameshcorbett left a comment •

edited

Loading

jameshcorbett Feb 5, 2025 •

edited

Loading

cmoussa1 commented Feb 5, 2025 •

edited

Loading