plugin: add configurable priority factors via TOML `conf.update` #295

cmoussa1 · 2022-11-07T16:49:50Z

Background

While working on #291, I found it hard to add a new factor to the priority plugin without having to change all of the sharness tests that checked for a specific priority number. A great suggestion was made that perhaps the plugin could make use of the conf.update callback topic, which would allow for configuration of values in a TOML file that can be extracted in the plugin and stored for future use. This would be especially useful for the integer weights of the various factors used in the priority calculation of a job for a couple of reasons:

it would make adding new factors easier so that existing tests do not have to be adjusted (since the new factor can be set to 0 in these tests).
it would allow an admin to define and set the weights for the factors used in job priority calculation, adjusting for whatever is most appropriate for the center during use, instead of using hard-coded weights defined in the plugin.

This PR adds TOML configuration support for the priority factors to the multi-factor priority plugin. The conf_update_cb () callback that is added to the plugin looks for a [priority_factors] section in the config file where the various priority factors are defined along with their associated integer weights. It extracts these values and places them in a map, where the key is the name of the factor and the value is its integer weight. These weights are then used in the priority_calculation () helper function where job priority is calculated. Previously, this helper function was using hard-coded values for the weights of each factor.

Finally, a new sharness test is added where checks are made that the priority of a job can be adjusted depending on the [priority_factors] configuration in the TOML file, as well as ensuring that no configuration of the priority factors (as well as using the default urgency) results in a job just using default weights set in the plugin.

cmoussa1 · 2024-05-23T17:14:52Z

I've continued to make progress on adding the "age" factor to the priority plugin, and I think it would be helpful to have this PR reviewed first as the "age" factor stuff is built on top of these commits. :-) @grondo, if you have a chance later this week or next, could you give this a review please?

grondo

This looks pretty good! I just had a suggestion on the layout of the configuration to more closely match how other Flux subsystems are organized. Let me know what you think. Definitely open for discussion!

A flux-config-accounting(5) man page should probably be added as well documenting the new configuration parameters and expanded when new configuration values are added.

grondo · 2024-05-23T19:56:37Z

src/plugins/mf_priority.cpp

+                                "{s?{s?{s?i, s?i}}}",
+                                "conf", "priority_factors",


Suggestion: put all flux-accounting values under one heading, maybe [accounting]?

Will all configurable priority factors actually be weights? If so, then maybe the sub-table should
just be factor-weights and the individual factor weights like fairshare, queue, age, etc.

So in the config file it would be

[accounting.factor-weights] fairshare = 1 age = 1

and so on?

That's a good suggestion, thanks! I've just pushed up my branch that makes this change to use a sub-table [accounting.factor-weights].

Problem: The priority plugin uses hard-coded values for the weights associated with each factor used to calculate a job's priority. Add a new callback to the plugin that extracts information pertaining to the priorities' respective weights used in the multi-factor priority calculation. Place the weight information in a map where the key is a string of the factor's name and the value is the factor's associated integer weight.

Problem: The priority_calculation () function uses hard-coded values for its calculation of a job's priority, but the plugin can read from a configurable TOML file now via the conf.update callback. Use the values located in the priority_weights map. If no values are found in a conf.update, the values for the factors located in the priority_weights map will be -1, so in this case, just use default values.

Problem: There exist no tests for updating the plugin via conf-update. Add some tests.

cmoussa1 · 2024-05-23T21:28:02Z

Thanks for the review @grondo!

A flux-config-accounting(5) man page should probably be added as well documenting the new configuration parameters and expanded when new configuration values are added.

Sure thing, I'd be happy to document this. Should I add that manpage to the doc folder in flux-accounting in this PR?

grondo · 2024-05-23T21:42:57Z

Sure thing, I'd be happy to document this. Should I add that manpage to the doc folder in flux-accounting in this PR?

Yeah, are there no manpages yet for flux-accounting?

We should start with this one. It should probably go in /doc/man5/flux-config-accounting.rst. You could probably borrow the setup from flux-sched which I think has the same structure.

cmoussa1 · 2024-05-23T21:46:49Z

Not yet, but I'll go ahead and take a crack at adding one for the config parameters proposed in this PR. 👍

cmoussa1 · 2024-05-23T22:03:38Z

I just pushed up a commit to add a new manpage to the doc/ folder - let me know what you think and if you'd like to see anything else added!

grondo · 2024-05-23T22:08:58Z

Great! Unfortunately I think some infrastructure is still needed to build and install the man page. I think you could check out flux-sched for the requirements, but I can try to help later

cmoussa1 · 2024-05-24T16:44:46Z

Thanks @grondo! I've started trying to copy over the infrastructure from flux-core and flux-sched to enable building and installing the man pages (I've pushed up another commit that you'll probably notice is failing 😉 ). I'm going to try and keep hacking away at this to see if I can get it to build successfully

cmoussa1 · 2024-05-24T21:42:52Z

OK, after a lot of tinkering I think I was finally able to get the man pages infrastructure set up. 😆 The last two commits should have all of the required files added and modified to be able to build and install the documentation for the priority plugin configuration, which can be viewed with man flux-config-accounting.

grondo · 2024-05-24T23:15:50Z

doc/man5/flux-config-accounting.rst

+The flux-accounting priority plugin can be configured to assign different
+weights to the different factors used when calculating a job's priority.
+Assigning a higher weight to a factor will result in it having more
+influence in the calculated priority for a job.


Maybe this should mention the default weight for factors that are not set in the config file?

Good suggestion, thanks. I've just force-pushed up a fix that adds a table which lists the default weights for factors if they are not configured.

Problem: There is no documentation for configuring the different factors used in calculating a job's priority. Add a new manpage to the doc/ folder that outlines the configuration process for the various factors used in the priority plugin.

Problem: The am_check_pymod.m4 file in flux-accounting is out-of-date compared to flux-core's, which results in a failure to run ./configure with --enable-docs set and the introduction of man pages to flux-accounting. Update am_check_pymod.m4 to match flux-core's.

Problem: flux-accounting has no way to build and install its man pages. Copy over the docs infrastructure from flux-sched and flux-core to be able to build the manpages for flux-accounting.

grondo

Great! LGTM!

cmoussa1 · 2024-05-28T15:00:20Z

Thanks! I'll set MWP here then

codecov · 2024-05-28T15:07:55Z

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.34%. Comparing base (2f811cf) to head (2485a88).
Report is 27 commits behind head on master.

❗ Current head 2485a88 differs from pull request most recent head da285a9

Please upload reports for the commit da285a9 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   83.30%   83.34%   +0.04%     
==========================================
  Files          23       23              
  Lines        1545     1555      +10     
==========================================
+ Hits         1287     1296       +9     
- Misses        258      259       +1

Files	Coverage Δ
src/plugins/mf_priority.cpp	`83.24% <91.66%> (+0.19%)`	⬆️

cmoussa1 added the new feature new feature label Nov 7, 2022

cmoussa1 force-pushed the add.factor.config branch 2 times, most recently from 770620a to e49df81 Compare November 8, 2022 17:14

garlick mentioned this pull request Nov 8, 2022

idea: allow priority function to be expressed in TOML config #296

Open

cmoussa1 changed the title ~~[WIP] plugin: add configurable priority factors via TOML conf.update~~ plugin: add configurable priority factors via TOML conf.update Nov 8, 2022

cmoussa1 marked this pull request as ready for review November 8, 2022 22:28

cmoussa1 requested a review from grondo November 8, 2022 22:28

cmoussa1 force-pushed the add.factor.config branch from e49df81 to 8c19d56 Compare November 14, 2022 17:44

cmoussa1 mentioned this pull request Nov 28, 2022

[WIP] plugin: add "age" factor to priority calculation #297

Closed

cmoussa1 force-pushed the add.factor.config branch from 8c19d56 to 48dcdf6 Compare November 29, 2022 15:55

cmoussa1 mentioned this pull request Dec 5, 2022

accounting: write docs about configuring priority factor weights in TOML file #502

Closed

cmoussa1 changed the title ~~plugin: add configurable priority factors via TOML conf.update~~ [WIP] plugin: add configurable priority factors via TOML conf.update Apr 11, 2023

cmoussa1 force-pushed the add.factor.config branch from 48dcdf6 to 1f9ac99 Compare March 20, 2024 17:21

cmoussa1 force-pushed the add.factor.config branch from 1f9ac99 to 2485a88 Compare May 16, 2024 18:30

cmoussa1 changed the title ~~[WIP] plugin: add configurable priority factors via TOML conf.update~~ plugin: add configurable priority factors via TOML conf.update May 20, 2024

grondo reviewed May 23, 2024

View reviewed changes

cmoussa1 added 3 commits May 23, 2024 14:19

t: add tests for plugin config update

22cf1a5

Problem: There exist no tests for updating the plugin via conf-update. Add some tests.

cmoussa1 force-pushed the add.factor.config branch from 2485a88 to 22cf1a5 Compare May 23, 2024 21:23

cmoussa1 force-pushed the add.factor.config branch 2 times, most recently from cfabd1a to 5a496dd Compare May 24, 2024 21:13

cmoussa1 force-pushed the add.factor.config branch 2 times, most recently from 708370d to e4ef5df Compare May 24, 2024 21:34

grondo reviewed May 24, 2024

View reviewed changes

cmoussa1 added 3 commits May 25, 2024 10:08

doc: add manpage for priority factors

264ff7c

Problem: There is no documentation for configuring the different factors used in calculating a job's priority. Add a new manpage to the doc/ folder that outlines the configuration process for the various factors used in the priority plugin.

doc: add infrastructure for building man pages

da285a9

Problem: flux-accounting has no way to build and install its man pages. Copy over the docs infrastructure from flux-sched and flux-core to be able to build the manpages for flux-accounting.

cmoussa1 force-pushed the add.factor.config branch from e4ef5df to da285a9 Compare May 25, 2024 17:08

grondo approved these changes May 25, 2024

View reviewed changes

cmoussa1 added the merge-when-passing label May 28, 2024

mergify bot merged commit dfffcc4 into flux-framework:master May 28, 2024
11 checks passed

cmoussa1 mentioned this pull request May 28, 2024

priority plugin: implement custom configuration for factor weights #128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin: add configurable priority factors via TOML `conf.update` #295

plugin: add configurable priority factors via TOML `conf.update` #295

cmoussa1 commented Nov 7, 2022 •

edited

Loading

cmoussa1 commented May 23, 2024

grondo left a comment

grondo May 23, 2024

cmoussa1 May 23, 2024

cmoussa1 commented May 23, 2024

grondo commented May 23, 2024

cmoussa1 commented May 23, 2024

cmoussa1 commented May 23, 2024

grondo commented May 23, 2024

cmoussa1 commented May 24, 2024

cmoussa1 commented May 24, 2024

grondo May 24, 2024

cmoussa1 May 25, 2024

grondo left a comment

cmoussa1 commented May 28, 2024

codecov bot commented May 28, 2024

plugin: add configurable priority factors via TOML conf.update #295

plugin: add configurable priority factors via TOML conf.update #295

Conversation

cmoussa1 commented Nov 7, 2022 • edited Loading

Background

cmoussa1 commented May 23, 2024

grondo left a comment

Choose a reason for hiding this comment

grondo May 23, 2024

Choose a reason for hiding this comment

cmoussa1 May 23, 2024

Choose a reason for hiding this comment

cmoussa1 commented May 23, 2024

grondo commented May 23, 2024

cmoussa1 commented May 23, 2024

cmoussa1 commented May 23, 2024

grondo commented May 23, 2024

cmoussa1 commented May 24, 2024

cmoussa1 commented May 24, 2024

grondo May 24, 2024

Choose a reason for hiding this comment

cmoussa1 May 25, 2024

Choose a reason for hiding this comment

grondo left a comment

Choose a reason for hiding this comment

cmoussa1 commented May 28, 2024

codecov bot commented May 28, 2024

Codecov Report

plugin: add configurable priority factors via TOML `conf.update` #295

plugin: add configurable priority factors via TOML `conf.update` #295

cmoussa1 commented Nov 7, 2022 •

edited

Loading