PP-388: Allow mom hooks to accumulate resources_used values

Developers
21

If a goal of the RFE is to detect errors (e.g., if one node did not report back), a better way to address that would be out-of-band (e.g., throw an exception and/or pass an explicit error object).

Again, hook writers get the best of both worlds (with and without MOM names) if the design allows hook writers to decide. If hook writers want to have MOM names (and want to use them for some type of error detection), they can simply include them in the JSON output. If not, then they have the flexibility to create a uniform output regardless of the number of MOMs a job happens to be scheduled upon.

22

error detection is one of the reasons for this the other is to clearly see per node reporting. If we throw an exception then users/admins cannot easily determine which node is having issues. I also, am not seeing how the hook writer gets the best of both worlds by providing/not providing the mom name as a key in the dictionary. If I understand you correctly, you are suggesting that we return a list of strings

resources_used.my_res = [“node1=string1”,“node2=string2”,“node4=string4”,…] notice that node 3 does not return a value

I am not sure how that is any better/worse for which ever application is trying to consume that value vs

resources_used.my_res = {“nodes1”:“string1”,“node2”:“string2”,“node3”:null,“node4”:“string4”}

Either way you have to loop through a list or a dictionary and both are straight forward to loop through. Am I missing something?

23

Could you provide a code example of how the error facility would be used? It seems like it would require a non-trivial piece of code (that would need to be included in every hook that uses this facility) versus having PBS supply an appropriate error (along with a list of MOMs that did not report values) in case of an error.

And, again, nothing prevents MOMs from including their MOM names. I am suggesting that it should be optional so that simple dictionaries of: { rank0 : 10.2, rank1 : 10.1, rank2, 9.8, … } is also a valid output (e.g., reporting kWh per MPI rank without having to strip out MOM names). Note that in this case, it’s also easy to see a few missing ranks and know that some MOMs did not provide a value.

24

After further discussion with the team we have decided to remove the check for when a node fails to report since this will be a corner case and we do not currently do this for our currently reported values of cput and mem.

As for forcing the MoM name to be the key for each string returned by the mom, we have decided to remove this requirement. This will require the hook writer to be responsible to return valid json in a key value pair. For example, if the desired output in the accounting logs is {“rank1”:4.5,”rank2”:5.0,”rank3”:13.2,”rank4”:2.3} and the job ran on 3 nodes this is what the hook writer would have to return

node1: ‘“rank1”:4.5,”rank2”:5.0’
node2: ‘“rank3”:13.2’
node3: ’“rank4”:2.3’

By removing this requirement however, it will open the hook writer to name collisions if they want to return socket or GPU name values since all nodes have a socket 0 for example. To get around this the hook writer can return the mom name as the key and a dictionary for the value. For example

node1: ‘“mom1”:{“GPU1”:4.1,”GPU2”:3.0}’
node2: ‘“mom2”:{“GPU1”:1.2}’
node3: ‘“mom2”:{“GPU1”:3.5}’

This would result in the following accounting log string

{“mom1”:{“GPU1”:4.1,”GPU2”:3.0},“mom2”:{“GPU1”:1.2},“mom3”:{“GPU1”:3.5}}

25

Thanks for reviewing and updating the design.

One part I really liked was the requirement for valid JSON – what about requiring the output from each MOM to be valid JSON (or a valid Python dictionary that easily maps to JSON), and having PBS merge it, e.g., the first example above would have MOM’s return:

node1 returns: '{ “rank1”:4.5, ”rank2”:5.0 }'
node2 returns: ‘{ “rank3”:13.2 }’
node3 returns: ’{ “rank4”:2.3 }’

The individual MOMs could verify JSON syntax (throwing appropriate exceptions).

When merging, before writing to the accounting log, PBS would perform the JSON equivalent of a Python’s dict(**node1, **node2, …) operation.

26

I recall from our earlier discussion that the rank was preferred as the primary key because it is guaranteed to be unique.

rank0: {mom: “node1”, cput: 1:15:18, mem: 2450MB, custom_size: 436MB}
rank1: {mom: “node1”, cput: 1:14:36, mem: 2130MB, custom_float: 3.25}
rank2: {mom: “node2”, cput: 1:16:01, mem: 2475MB, custom_string: “foo”}
rank3: {mom: “node2”, cput: 1:14:55, mem: 2197MB, custom_long: 12345}

No naming collisions with this model.

27

@billnitzberg: Having individual Moms validate the value as JSON-syntax is problematic since it is a value to a string resource, which can be of any string format. It would break compatibility of string resources if doing so.
Now MS is the only one that would know if the values are being aggregated, and currently mom is not Python aware. It’s the separate pbs_python command that mom calls to execute hook that is linked to Python. I’d rather not have the mom to now link to python library. I prefer keeping the python functionality separare and also to not bloat mom. As it currently stands, mom will see string values say from momA, from momB, etc …which will be combined as {,,…} and hook writer just needs to make sure the string values can be dictionary entries. We can put in our doc an advise to the hook writer on formatting the values. Since we’re also offering flexibility, what if a site does not want to do json-syntax but something else, so we do not want to limit that…

28

I like this idea. However, as soon as you remove the mom name as the key for each reported node you then have no idea what the hook writer will return (i.e. dictionary, list, key:value pair, or string). You could individually check what each mom returns, however there is no guarantee that they will return something that will be valid json once aggregated by the mother superior.

The rank is not preferred over any other unique key (i.e. mom name).

29

@Jon: OK, looking at the JSON definition (http://www.json.org), there’s no dictionary (my bad). There are two types that make sense – an object and an array – both would work for the output in the accounting log. The advantage of the array is that it is an ordered list (so there is no requirement for keys, nor for keys to be unique, but one can easily include keys). The object, on the other hand, has the “set” property, but nicely maps to a Python dictionary.

If we want the output in the accounting logs to be a JSON object, then having each MOM return a JSON object and merging them makes sense, and is easily doable and testable within the MOMs.

If we want to support more free-form outputs, then simply requiring each MOM to return a JSON value and combining them into an array in the accounting log would be a bit more flexible.

I’m a bit torn between the object and array. Similar outputs in the accounting logs are strictly ordered (exec_vnode), which points to using JSON arrays, but I think the JSON object (aka dictionary) is a nice fit with how people use JSON with Python.

In any case, there is no issue verifying that what each MOM returns is a valid JSON value (beyond the point that @bayucan makes regarding additional engineering effort).

30

@billnitzberg: I believe that we should support the JSON object since it fits in nicely with Python. As for validating the JSON output we will leave that up to the hook writer for now since we are giving them the extra flexibility to return what ever they want as long as it maps to a key:value pair. Also, it is not clear to me what we should do if they don’t return a valid json output. If you would like to file a ticket with how you would like it to behave we when errors are encountered we can look at adding it in the future.

31

@Jon: OK, I think I understand now – I had thought the proposal was to add JSON, so that MOMs could return JSON and JSON would be output into the accounting logs and would be available via qstat. I really love this idea. However, I now understand that is not what is being proposed. If I understand correctly, the proposal is that each MOM returns a string and those strings are captured into a single JSON-like object (that is only 1 level deep) with key’s corresponding to the MOM names. In particular, there is no way to represent any JSON value other than a single JSON object with keys and non-complex (e.g., string) values, so nested objects would not be supported.

To me, it makes sense to have a JSON format, but if PBS is not going to validate the syntax, then I suggest removing JSON entirely from the equation. In other words, every MOM would return a simple resource value string, and they would be concatenated. As long as PBS defines the order of concatenation, then it is possible for a hook writer to output legal JSON if so desired, but the onus would be completely on the hook writer. (And, of course, getting the leading and trailing elements, including the commas correct is a bit of a hook-writer’s hassle, but it is possible).

I would rather see PBS actually implement a JSON value type, do proper validation, and all, but I gather from your “file a ticket … we can look at adding it in the future” that the additional effort is not available at this time.

One additional note: While investigating about this, I did find the following in the PBS Reference Guide which disallows both types of quotes in a resource value. If PBS is going to start allowing this in the accounting logs, that may mess up some parsers that depend on the published interface. (At the very least, a big warning should be given in the release notes about potentially breaking backward compatibility.) From the PBS Reference Guide in the Formats section:

String (resource value)
Any character, including the space character.
Only one of the two types of quote characters, " or ', may appear in any given value.
Values:[_a-zA-Z0-9][[-_a-zA-Z0-9 ! " # $ % ́ ( ) * + , - . / : ; < = > ? @ [ \ ] ^ _ ’ { | } ~] …]
String resource values are case-sensitive.

To summarize: I suggest either going full JSON or going full “strings with concatenation and nothing else”. Going partly JSON (as proposed) will make any future extensions to incorporate full JSON into PBS very messy to get right.

32

Thinking about this a bit more, wanting to allow full JSON, what about…

Having PBS recognize JSON syntax and treat it special. When Mother Superior is merging the string values, it can test for valid JSON syntax. If all the values are valid JSON objects, then they are merged into a single JSON object; ditto for JSON arrays. If they are not all either JSON objects or JSON arrays, then the PBS strings are simply concatenated.

This easily allows hook writers to create full JSON objects (or arrays) that flow into the accounting logs and qstat output, but doesn’t require any additional work on the sister MOMs (although it does require a bit of additional work on Mother Superior).

It still would have ramifications if PBS adds full JSON types in the future, as PBS string types will have this extra weird behavior… not sure I like that, but…

Also still need to take care of the quoting in the accounting logs (but that’s true no matter what).

33

After our discussion, I believe that the correct approach is to require a valid json object to be returned for a string resource being returned for resources_used. If they return a non valid json object then the MS will not report back a value causing no value to be written in the accounting logs. If this happens then the MS will log what was returned for the hook writer to debug. However, If all of the mom’s return a valid json object then MS will join all of the separate json objects into a single json object and that will be reported in the accounting log.

34

Good design discussion – thanks for all the interactions. I like the latest proposal, as I think it will provide enough flexibility for hook writers today, without adding too much “design-debt” (that will hinder PBS in the future).

Just to double-check, in “MS will join all of the separate json objects into a single json object”, Join means perform a union, e.g., { “a”:1, “b”:2 } joined with { “c”:1 } becomes { “a”:1, “b”:2, “c”:1 }. Correct?

35

The behavior you described is what will happen, Bill. It’s correct.

36

I’ve made another update to the design document based on recent discussions. The link is as follows:

https://pbspro.atlassian.net/wiki/display/PD/PP-388%3A+Allow+mom+hooks+to+accumulate+resources_used+values

The use of mom hostnames as keys has been dropped. Now each value obtained from sister moms must be of JSON format, and mother superior mom will accumulate the string resource values by merging (union) the values into one JSON string.

37

Added an update to the EDD, adding interface 2, which will be a consequence of the implementation.

https://pbspro.atlassian.net/wiki/display/PD/PP-388%3A+Allow+mom+hooks+to+accumulate+resources_used+values

38

Updated the EDD again…it’s actually not an interface change but an added note, and also just referring to special string_array type in PBS.

39

Thanks for adding the note Al. It looks good to me.