RFC21: add new PRIORITY state #271

garlick · 2020-11-10T21:32:07Z

As discussed in flux-framework/flux-core#3311, this PR adds a new PRIORITY job state that a job passes through when it is assigned its initial priority by a job manager priority plugin.

This should make integration of a priority plugin a bit easier, and also provide some user visibility in case the plugin takes a long time to assign the initial priority (for example if it has to fetch data from flux-accounting before it can answer).

Events normally drive job state transitions. I added a prioritize event for this purpose, but footnoted it with the caveat that, unlike the other primary events, it will not be posted to the job eventlog. This is because we surely do not want to log the repeated priority adjustments of a fair share priority plugin for posterity, and a fresh priority can easily be recalculated if the instance restarts with jobs in SCHED state.

That does mean that a job can regress from SCHED to PRIORITY state on an instance restart. It does not mean that the job repeatedly does that as the priority is updated, since the job may have an outstanding alloc request to the scheduler that we do not want to cancel every time the priority is updated. (Updating the scheduler's priority info will be the subject of another PR to RFC 27)

garlick · 2020-11-10T21:42:03Z

Oh man, no idea what this means. Readthedocs is working fine on my laptop (make html). I can't tell if this even involves the rfc I changed. Not?

Successfully built pyyaml pyrsistent
ERROR: Exception:
Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 228, in _main
    status = self.run(options, args)
  File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/cli/req_command.py", line 182, in wrapper
    return func(self, options, args)
  File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/commands/install.py", line 379, in run
    requirement_set
  File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 191, in get_installation_order
    weights = get_topological_weights(graph)
  File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 242, in get_topological_weights
    assert len(weights) == len(graph)
AssertionErrorr

SteVwonder · 2020-11-10T23:44:39Z

Looks like we smacked into a pip dependency resolution bug: pypa/pip#9031

I don't quite grok their proposed solution of removing Sphinx as a requirement. I'm guess they mean Sphinx is already included as an intermediate/implicit dependency thanks to sphinx-rtd-theme and/or sphinxcontrib-spelling. So maybe dropping sphinx from requirements.txt will work for us 🤷 .

Unfortunately, I don't see a way to have RTD not use the 2020 resolver 🤦

SteVwonder · 2020-11-10T23:47:07Z

@garlick : if you run pip install --upgrade pip and then pip install --use-feature 2020-resolver --exists-action=w --no-cache-dir -r requirements.txt, it might reproduce locally

garlick · 2020-11-11T00:34:52Z

Thanks for that hint. It didn't reproduce but for some reason some extraneous warnings went away and I now noticed that one of the JSON examples is called out as invalid JSON and could not be syntax highlighted. I tacked on a commit to fix that.

SteVwonder · 2020-11-11T03:37:50Z

t didn't reproduce

Darn. The only thing that I can think to try currently is to drop the explicit sphinx from the requirements.txt and let it just be implicitly picked up as a dependency from the other packages. That seemed to work for the other people in thread.

garlick · 2020-11-11T15:41:59Z

Ah thanks. Well that seems to have worked. Are we OK with that change getting merged or does this need more thought?

On re-reviewing this proposed change, one other thing I should have pointed out for discussion is I defined the priority as a double with a range of 0-1.0. My thought was that this value will become the only value used to order the queue, with t_submit and administrative priority becoming factors that are input to a multi-factor priority plugin, probably with a default "builtin plugin" that only considers those factors.

Would it be better to go with an integer value here? how about the range?

grondo

Just caught one typo on the first pass through.

grondo · 2020-11-11T15:58:26Z

spec_21.rst

+Prioritize Event
+^^^^^^^^^^^^^^^^
+
+Job has been (re-)assigned a priority by the job shell priority plugin:


Suggested change

Job has been (re-)assigned a priority by the job shell priority plugin:

Job has been (re-)assigned a priority by the job-manager priority plugin:

I'll fix that.

Also, based on our discussions offline about the confusion of the PRIORITY state vs priority event that would affect it indirectly, not directly, I think I'll propose changing the priority event to admin-priority and prioritize to priority.

Problem: sphinx build fails with the following assertion Successfully built pyyaml pyrsistent ERROR: Exception: Traceback (most recent call last): File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 228, in _main status = self.run(options, args) File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/cli/req_command.py", line 182, in wrapper return func(self, options, args) File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/commands/install.py", line 379, in run requirement_set File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 191, in get_installation_order weights = get_topological_weights(graph) File "/home/docs/checkouts/readthedocs.org/user_builds/flux-rfc/envs/271/lib/python3.7/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 242, in get_topological_weights assert len(weights) == len(graph) AssertionErrorr This is apparently a pip dependency error that some users have worked around in the case of sphinx by removing the explicit requirement. Let's give that a try.

Problem: sphinx cannot parse one of the JSON examples: spec_21.rst:245: WARNING: Could not lex literal_block as "json". Highlighting skipped. Add a missing double quote.

Problem: the TBD entries under state synchronization are not useful. Also there's a missing period following a footnote reference. Drop the TBD entries and add the missing period.

Problem: a PRORITY state is being added, and the name of the event that changes the administrative priority is called "priority" which will lead to confusion. Rename the priority event to admin-priority. A new priority event will be defined that affects the overall priority in a subsequent commit.

cmoussa1 · 2020-11-11T17:33:43Z

On re-reviewing this proposed change, one other thing I should have pointed out for discussion is I defined the priority as a double with a range of 0-1.0. My thought was that this value will become the only value used to order the queue, with t_submit and administrative priority becoming factors that are input to a multi-factor priority plugin, probably with a default "builtin plugin" that only considers those factors. Would it be better to go with an integer value here? how about the range?

If we were to model our multi-factor priority calculation in a similar manner to Slurm, I believe they define their job priority value as an int that ranges between 0 and 4294967295. Here is where they define their formula. All of the factors used in this calculation are floating point numbers that range from 0.0 to 1.0. I hope this helps!

chu11 · 2020-11-11T17:52:58Z

spec_21.rst

@@ -195,6 +195,20 @@ Example:
   {"timestamp":1552593348.073045,"name":"submit","context":{"priority":16,"userid":5588,"flags":0}}


+Depend Event


commit message typo "mising" -> "missing"

chu11 · 2020-11-11T18:07:08Z

On re-reviewing this proposed change, one other thing I should have pointed out for discussion is I defined the priority as a double with a range of 0-1.0. My thought was that this value will become the only value used to order the queue, with t_submit and administrative priority becoming factors that are input to a multi-factor priority plugin, probably with a default "builtin plugin" that only considers those factors.

my first thought is that when priority plugin developers are debugging, it may be visually easier to see integers vs. floats. Like I may not want to see "0.83738937" in logs all over the place.

garlick · 2020-11-11T18:08:23Z

Thanks @cmoussa1! An integer with range of 0 to 4294967295 sounds completely reasonable to me, and presumably it will make comparing slurm and flux priority algorithms slightly easier if we don't introduce arbitrary changes here.

garlick · 2020-11-11T18:20:49Z

Forced a push with the priority redefined as an integer from 0 - 2^32-1, and fixed the spelling error @chu11 pointed out in my commit message. I also reworded that commit message, and added one to the main commit that adds the new state.

chu11 · 2020-11-11T18:31:04Z

an aside, should we update all language in flux-core (and sched?) about priority to "administrative priority"? e.g. flux job priority, output from flux-jobs, etc.

On the one hand, consistency. On the other hand, presumably at the user/admin level, "administrative priority" is the only priority they know, so it shouldn't matter.

garlick · 2020-11-11T18:33:11Z

On the other hand, presumably at the user/admin level, "administrative priority" is the only priority they know, so it shouldn't matter.

This is the way I was leaning. Where it might get confusing is if we want to let users see what their current (caculated) priority value is. What do we call that?

chu11 · 2020-11-11T18:35:46Z

Where it might get confusing is if we want to let users see what their current (caculated) priority value is. What do we call that?

queue priority?

garlick · 2020-11-11T18:36:39Z

Ah sure, that makes sense. So for example if we want flux job info to be able to list it in a custom formatted output, {queue-priority}. I like it.

grondo · 2020-11-11T20:27:06Z

The PRIORITY state should also be added to the list for the ACTIVE virtual state

Add a new PRIORITY state, between DEPEND and SCHED. A job will remain in PRIORITY state while its priority is being calculated by a job manager priority plugin. In many cases this may be instantaneous, but we leave open the possibility that a plugin may need to contact another service to obtain user data before assigning a priority. If this service is slow or unresponsive, the amount of time spent in PRIORITY state will provide some visibility into what is going on. A 'priority' event is posted when the priority is assigned, and later when the priority is updated. This event is not posted to the job eventlog to avoid the extra noise, but would appear in the job manager journal so that external services like job-info can reorder their queue.

Problem: A depend event that causes the job to transition out of DEPEND state is discussed, and is present in the state diagram, but is not defined. Define the depend event, with empty context matching the current implementation.

grondo

LGTM! Thanks!

garlick · 2020-11-11T22:14:04Z

Thanks, I'll set MWP

garlick force-pushed the rfc21_priority branch from 1f1d147 to 3dab6b4 Compare November 10, 2020 22:41

grondo reviewed Nov 11, 2020

View reviewed changes

garlick added 5 commits November 11, 2020 08:37

rfc21: fix invalid JSON example

de4f347

Problem: sphinx cannot parse one of the JSON examples: spec_21.rst:245: WARNING: Could not lex literal_block as "json". Highlighting skipped. Add a missing double quote.

rfc21: drop TBD state sync, add missing period

83db73e

Problem: the TBD entries under state synchronization are not useful. Also there's a missing period following a footnote reference. Drop the TBD entries and add the missing period.

rfc21: add PRIORITY state to diagram

64398c0

garlick force-pushed the rfc21_priority branch 2 times, most recently from f9045dc to f8650e6 Compare November 11, 2020 17:14

chu11 reviewed Nov 11, 2020

View reviewed changes

garlick force-pushed the rfc21_priority branch from 6ef85ce to 3e5c61c Compare November 11, 2020 18:19

garlick added 2 commits November 11, 2020 13:16

rfc21: add missing depend event

66b93d4

Problem: A depend event that causes the job to transition out of DEPEND state is discussed, and is present in the state diagram, but is not defined. Define the depend event, with empty context matching the current implementation.

garlick force-pushed the rfc21_priority branch from 3e5c61c to 66b93d4 Compare November 11, 2020 21:16

grondo approved these changes Nov 11, 2020

View reviewed changes

garlick added the merge-when-passing label Nov 11, 2020

mergify bot merged commit bddb3c5 into flux-framework:master Nov 11, 2020

garlick deleted the rfc21_priority branch November 14, 2020 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC21: add new PRIORITY state #271

RFC21: add new PRIORITY state #271

garlick commented Nov 10, 2020

garlick commented Nov 10, 2020

SteVwonder commented Nov 10, 2020

SteVwonder commented Nov 10, 2020

garlick commented Nov 11, 2020

SteVwonder commented Nov 11, 2020

garlick commented Nov 11, 2020

grondo left a comment

grondo Nov 11, 2020

garlick Nov 11, 2020

cmoussa1 commented Nov 11, 2020

chu11 Nov 11, 2020

chu11 commented Nov 11, 2020

garlick commented Nov 11, 2020

garlick commented Nov 11, 2020

chu11 commented Nov 11, 2020 •

edited

Loading

garlick commented Nov 11, 2020

chu11 commented Nov 11, 2020

garlick commented Nov 11, 2020

grondo commented Nov 11, 2020

grondo left a comment

garlick commented Nov 11, 2020

	Job has been (re-)assigned a priority by the job shell priority plugin:
	Job has been (re-)assigned a priority by the job-manager priority plugin:

		@@ -195,6 +195,20 @@ Example:
		{"timestamp":1552593348.073045,"name":"submit","context":{"priority":16,"userid":5588,"flags":0}}


		Depend Event

RFC21: add new PRIORITY state #271

RFC21: add new PRIORITY state #271

Conversation

garlick commented Nov 10, 2020

garlick commented Nov 10, 2020

SteVwonder commented Nov 10, 2020

SteVwonder commented Nov 10, 2020

garlick commented Nov 11, 2020

SteVwonder commented Nov 11, 2020

garlick commented Nov 11, 2020

grondo left a comment

Choose a reason for hiding this comment

grondo Nov 11, 2020

Choose a reason for hiding this comment

garlick Nov 11, 2020

Choose a reason for hiding this comment

cmoussa1 commented Nov 11, 2020

chu11 Nov 11, 2020

Choose a reason for hiding this comment

chu11 commented Nov 11, 2020

garlick commented Nov 11, 2020

garlick commented Nov 11, 2020

chu11 commented Nov 11, 2020 • edited Loading

garlick commented Nov 11, 2020

chu11 commented Nov 11, 2020

garlick commented Nov 11, 2020

grondo commented Nov 11, 2020

grondo left a comment

Choose a reason for hiding this comment

garlick commented Nov 11, 2020

chu11 commented Nov 11, 2020 •

edited

Loading