Add memory / CPU usage properties to pipeline nodes

[marthacryan]

Fixes #1188. Adds three fields to the properties editor in the pipeline editor to specify CPU, memory, and GPU usage for each node. The fields are optional. Memory has two fields: one to indicate a number, and another to indicate the unit of measurement (such as Gi, P, etc.) through a dropdown. There is validation to prevent the user from adding any negative values or any values over 99 for CPU and GPU (99 was chosen somewhat arbitrarily).

Here's how they look with the small properties editor:

And with the large properties editor:

This is the dropdown for memory:

This is the validation message:

There is a bug on the canvas side that I want to investigate - the validation considers an empty field to be 0, so it shows the validation error above when the input field is empty.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

[elyra-bot]

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link:

[lresende]

Could we simplify and always use GB (as in updating the label) and that allows us to remove the unit field? Also, these will most of the time be single digits except for memory that would be maybe 2 or 3 digits, so we could try to use less real estate for the fields?

[marthacryan]

Could we simplify and always use GB (as in updating the label) and that allows us to remove the unit field? Also, these will most of the time be single digits except for memory that would be maybe 2 or 3 digits, so we could try to use less real estate for the fields?

@lresende I can definitely just use GB to remove the unit field. I tried moving them to just one line and in the small size it looks a little crowded if you include GB in the title:

I could spread them over 2 lines:

Or it also looks a little less crowded if you move the "GB" out of the title of the RAM:

[kevin-bates]

Could we place the "PUs" adjacent to each other? I understand RAM and CPU may be on the same line, depending on the window's width, but in the case where they're all vertically or horizontally aligned, I think having CPU and GPU adjacent is better from a UX perspective.

[marthacryan]

Updated look:

Does anyone have feedback / suggestions for the descriptions?

[akchinSTC]

that looks good!

[lresende]

Just let's be careful with decimals around GPUs (and maybe others) as in the past GPUs factions could not be assigned to a pod.

[marthacryan]

@lresende @akchinSTC I checked with Alan before allowing all fields to be doubles, and I believe he checked to make sure it'd work

[lresende]

This page says:

'Each container can request one or more GPUs. It is not possible to request a fraction of a GPU.'

[marthacryan]

This page says:

'Each container can request one or more GPUs. It is not possible to request a fraction of a GPU.'

Oh ok so I'll set the GPU field to integer (unless Alan disagrees?) but keep the other 2 fields as doubles?

[marthacryan]

Realized that it isn't possible to ensure that a number field is strictly an integer - there isn't a way around it at the moment but I can add a comment in the description and I added an issue in canvas.

[lresende]

This LGTM now. @akchinSTC how are we planning to integrate the backend changes? Will it be part of this PR?

[akchinSTC]

@lresende - yes im going to be adding the backend changes using this as a base.

[akchinSTC]

Dependent kfp-notebook PR here : elyra-ai/kfp-notebook#78

[lresende]

Thanks @akchinSTC, I looked in the backend code and I was wondering how are we falling back to the current behavior of not sending these parameters if not set/selected by the user.

[kevin-bates]

I think this might warrant a pipeline version increment. The problematic case being a new pipeline file with these parameters set, getting used by an older version - which would then save the pipeline w/o these values (I think).

[akchinSTC]

Used a new pipeline file with cpu/mem resources defined, on Elyra 1.x. Upon saving the properties in the node, I've confirmed it will not overwrite/delete the resource values.

[lresende]

A few issues:

• Looks like when you set a value to a field (e.g. gpu) and then reset (e.g. select + delete) the field values are being set as null instead of something like an empty string or no field at all.
• Looks like you can use the little buttons to increase/decrease the values, but it's going below 0, which it should not.

And things working well:

• I tested the pipeline being edited by the pipeline editor with these changes and see the fields there, going back to an old version of the editor and editing the pipeline does not seem to cause any side effects or erase the fields from the pipeline file, which means we don't need a version change for the pipeline.

I will continue testing the runtime integration tomorrow, as the KFP server I was using was down when I was testing it.

[marthacryan]

• Looks like when you set a value to a field (e.g. gpu) and then reset (e.g. select + delete) the field values are being set as null instead of something like an empty string or no field at all.

I think it wouldn't be possible to set it to empty string because it's a number field. I can add logic to check if the field is null and delete it from the properties (when applying properties) if that helps the backend, but would it be different in terms of the python code to have a field set to null vs. the field not existing? @akchinSTC

• Looks like you can use the little buttons to increase/decrease the values, but it's going below 0, which it should not.

Unfortunately I think this is a behavior on the canvas side (I just reached out to them to make sure), I don't think there's a way around that at the moment, but it does show a validation error immediately when you press the button and it becomes an invalid field. I added an issue and they said that the behavior we're looking for already exists in the component they're using, they just have to add it as an option, so it should be relatively quick.

[marthacryan]

Is there supposed to be a G at the end of this line?

[akchinSTC]

its a little weird I admit, looking for hands if we want to change it.
We could add another parameter to the template rendering function in the kfp processor to pass the value with G appended to it, in this case it was just quicker to add a G to the template.

[akchinSTC]

Tests are failing due to the kfp-notebook changes that this PR is dependent on, have not been merged and new package pushed to pypi. In order to test,

Dependent kfp-notebook PR here : elyra-ai/kfp-notebook#78

pull this PR and install locally.