Adding resource limits that can be selected from the UI

[paloma-rebuelta]

I have changed the backend to replace cpu and memory for cpu_request and memory_request, as well as adding cpu_limit and memory_limit. This has also been added to the UI.

These changes should fix the issue where the pipeline breaks when there are pre-defined argo resource limits that are lower than the requests, fixes #3168

milestone 4.0

I have tested this by adding two unit tests and building the code to run in a kubeflow server. I tested a pipeline with all the resource requests/limits combinations (including leaving it blank). I cannot run airflow pipelines in my setup but have also edited the code for these.

[paloma-rebuelta]

@lresende @kevin-bates @shalberd

[shalberd]

@romeokienzler @lresende maybe we should merge in Airflow 2.x related changes to processor airflow and airflow jinja2 template first before the changes here. @paloma-rebuelta in Airflow 2, a lot of changes happened and resource requests and limits are now specified a little differently, see my Airflow 2.x PR 3167

[kevin-bates]

Regarding the screen layout (thank you for including that), I find it more intuitive to have CPU request and limit on the same line, rather than CPU and Memory request, then CPU and Memory limit. I think keeping the "subject" on its own line would be better, and is also consistent relative to the CPU line (as its fields are on the same line as well). Thanks.

[paloma-rebuelta]

Regarding the screen layout (thank you for including that), I find it more intuitive to have CPU request and limit on the same line, rather than CPU and Memory request, then CPU and Memory limit. I think keeping the "subject" on its own line would be better, and is also consistent relative to the CPU line (as its fields are on the same line as well). Thanks.

done :)

[kevin-bates]

@romeokienzler @lresende maybe we should merge in Airflow 2.x related changes to processor airflow and airflow jinja2 template first before the changes here. @paloma-rebuelta in Airflow 2, a lot of changes happened and resource requests and limits are now specified a little differently, see my Airflow 2.x PR 3167

@shalberd - what is the ETA of the Airflow 2 PR - its been open for some time now. That PR speaks of really wanting limit settings and the changes to add CPU/Memory requests are trivial. In addition, this PR still supports Airflow 1x while I can't determine if your PR replaces that support or not (although I suspect it does).

As a result, I think this PR should take priority (assuming we don't require a migration), set the limit handling in place against existing user installations (preferrably with the release), then release support for Airflow 2x. If 2x support is a replacement, that's another issue worth discussing (if it hasn't been discussed already).

[kevin-bates]

@lresende, since this change adds a helpful feature (and is low risk IMO), I wonder if we should include it in 3.x and use Airflow 2 support as the trigger for 4.x. Are there other merged PRs that are only slated for 4.x? If we do decide to include this in 3.x we should probably adjust the changelog updates.

[shalberd]

@kevin-bates @lresende Happy New Year. I agree that this PR here should take precedence and is low risk, as adding optional limits is something that is extremely useful no matter what target runtime and is backward compatible. As for the Airflow PR: yes, after we have merged in this limits PR here, I will make the few modifications to the assembly of Airflow 2.x code, i.e. the limits fields in Airflow 2.x compatible syntax. My Airflow PR is intended not for backward compatibility, but for replacing Airflow 1.x support with Airflow 2.x support in version 4 of Elyra. I agree that we can make this limits PR here part of 3.x Elyra. Thanks again to @paloma-rebuelta for the great work on this here.

[paloma-rebuelta]

@kevin-bates @lresende Happy New Year. I agree that this PR here should take precedence and is low risk, as adding optional limits is something that is extremely useful no matter what target runtime and is backward compatible. As for the Airflow PR: yes, after we have merged in this limits PR here, I will make the few modifications to the assembly of Airflow 2.x code, i.e. the limits fields in Airflow 2.x compatible syntax. My Airflow PR is intended not for backward compatibility, but for replacing Airflow 1.x support with Airflow 2.x support in version 4 of Elyra. I agree that we can make this limits PR here part of 3.x Elyra. Thanks again to @paloma-rebuelta for the great work on this here.

@kevin-bates / @lresende Happy new year! I am happy to edit the milestone, is there a specific version of 3.x I should put? 3.15.1 or 3.16.0 maybe?

[shalberd]

@paloma-rebuelta we will make this part of release 4.0 (confirmed by @lresende) , looks good. I'll have a look at the code one more time, too, and test it. Having @kevin-bates approved it and your extensive discussions already make this seem mergeable.

[shalberd]

alos wondering about the env var minus GUI approach vs. env var and GUI approach discussed here before with @kevin-bates https://github.com/elyra-ai/elyra/issues/3168#issuecomment-1864855870

[kevin-bates]

@shalberd - regarding:

we will make this part of release 4.0 (confirmed by @lresende) , looks good. I'll have a look at the code one more time, too, and test it. Having @kevin-bates approved it and your extensive discussions already make this seem mergeable.

I'd still prefer we include this in a 3.x release since its independent of AF 2 support. I guess I'd prefer to see @lresende comment on my comment above regarding where this should be merged before concluding its only in 4.0.

alos wondering about the env var minus GUI approach vs. env var and GUI approach discussed here before with @kevin-bates https://github.com/elyra-ai/elyra/issues/3168#issuecomment-1864855870

I'm a big fan of envs as default values, but also feel that (for this application in particular) having a GUI presence is preferred. I believe the previous discussion assumed that the GUI work was not able to be done, so we'd start with envs, then, when a GUI is in place, use the previous env-based effort to represent defaults/preferences. Since @paloma-rebuelta will be receiving "extra credit" on this by actually introducing the GUI changes (thank you!), the env portion of things is a bit muted IMHO.

[shalberd]

I believe the previous discussion assumed that the GUI work was not able to be done, so we'd start with envs ... the env portion of things is a bit muted IMHO.

Agreed, just wanted to hear your perspective on this. We had customers on Github issues here that mentioned they have a requirement for limits to be set. So the only added perspective of an env in this new GUI approach would be: set a limit by factor * request if, and only if, no value is set in a node for a limit, across all nodes.

So, I'd say it's muted, but not entirely gone. However, we could cover that with a separate enhancement ticket and pull request, as it is kind of an extension for a special customer case.

[lresende]

@shalberd - regarding:

we will make this part of release 4.0 (confirmed by @lresende) , looks good. I'll have a look at the code one more time, too, and test it. Having @kevin-bates approved it and your extensive discussions already make this seem mergeable.

I'd still prefer we include this in a 3.x release since its independent of AF 2 support. I guess I'd prefer to see @lresende comment on my comment above regarding where this should be merged before concluding its only in 4.0.

I was suggesting we merge to the main branch and that I would push for the next release to be 4.0 with support for JupyterLab... if others are willing to maintain a 3.x branch I am open for it. At the moment I won't be able to manage 3.x and 4.x releases in parallel.

Feel free to merge this to main once you are happy with it @kevin-bates

[shalberd]

@kevin-bates let's hold off merging for a bit until we clear up the questions related to node editor limits field placement and cascading style sheets, as well as the question as to why for a single file "Submit as pipeline" does not show the new fields. I am trying out a little, too, but @paloma-rebuelta might be quicker to answer. We're almost there, thanks again, @paloma-rebuelta, the work on this PR is great.

all good, I forgot to do the make package-uistep, working nicely and as described

the only cosmetic thing I do not like so much is that the content on the Submit File to pipeline dialog is now pretty crowded

regardless of how wide or resized that popup / overlay window is

basically, there is no vertical space between the field columns anymore because it has to fit in the fixed size width of the whole canvas. Compare to before

@paloma-rebuelta @kevin-bates /lgtm

[kevin-bates]

if others are willing to maintain a 3.x branch I am open for it.

No can do either. So this will first appear in 4.0. Thanks for the response @lresende

[paloma-rebuelta]

Thanks @kevin-bates @shalberd and @lresende for getting this across 🥳

[shalberd]

you're welcome; this has been a long-standing wish of many people in the community, so your effort on this is extremely appreciated by not just us, I am sure.