policy: refactor options to allow custom

flux-framework / flux-sched

Fluxion Graph-based Scheduler

GNU Lesser General Public License v3.0

89 stars 41 forks source link

policy: refactor options to allow custom #1268

Open wihobbs opened 3 months ago

wihobbs commented 3 months ago

Problem: the current fluxion policy matching factory doesn't allow for customizable policies.

Add a custom option and refactor existing policies to behave similarly. Add unit testing for the string parsing convenience functions.

Outstanding to-do:

[ ] add an option in the scheduler config to actually pass a custom string in, and probably some validation too

wihobbs commented 3 months ago

And I know the testing and the code should be in separate commits... sigh (will fix)

garlick commented 3 months ago

PR title seems truncated?

wihobbs commented 3 months ago

Well, custom is a new type of policy that this PR is enabling. Or will enable eventually. Maybe "resource: refactor policy options to allow a user-defined policy" is more descriptive?

Note that this is still WIP and needs some config work and testing for the custom options. Happy to incorporate any feedback on the initial draft (and thanks to those who have already reviewed!), but it's going to stay in draft state until the code can:

[ ] accept a custom string with policy options
[ ] reject certain strings that are requesting invalid custom options, such as set_stop_on_k_matches=-100 and such
[ ] test some custom options and make sure they behave as expected
[ ] break out the commit history in a better way

wihobbs commented 2 months ago

Per discussion with Tom:

[ ] To handle custom policies, "don't validate, parse" -- parse the input on a delimiter first and then verify that each key is valid, give an error message if it isn't, possibly demote this to a warning based on a config option
[ ] Fluxion should issue a fatal warning if a policy named is not valid -- don't fall back on first (might already be done?)
[ ] set_stop_on_k_matches probably only takes a value of 1, however, we should test this
[ ] we should also think about having "node_centric" have a score factor of 10000, maybe we don't want to allow users to configure this, but maybe that number doesn't cover all our bases

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.1%. Comparing base (996f999) to head (2d176c4). Report is 18 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1268 +/- ## ========================================= - Coverage 75.3% 62.1% -13.3% ========================================= Files 111 51 -60 Lines 15300 6188 -9112 ========================================= - Hits 11531 3844 -7687 + Misses 3769 2344 -1425 ``` [see 99 files with indirect coverage changes](https://app.codecov.io/gh/flux-framework/flux-sched/pull/1268/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework)