ICRAR / daliuge

The DALiuGE Execution Engine
GNU Lesser General Public License v2.1
24 stars 7 forks source link

Liu 284 - Automatic deployment option detection #199

Closed pritchardn closed 1 year ago

pritchardn commented 2 years ago

This PR adds the capability to the translator allowing it to detect what deployment options a provided endpoint provides.

This is achieved by adding an additional rest-endpoint submission_method to the manager, lg_web and separately, OOD app returning a json containing what methods are available. The browser sends requests to the translator and provided manager URL whenever the deployment settings modal is opened.

There is a little complexity; the translator needs to test if the provided endpoint is available through the translator or directly from the browser, which for the translator case requires an additional request made to the manager URL - this means the manager will field two submission_method requests practically simultaneously.

Some quirks regarding the UI which we may or may not want to address before merging:

Some additional changes:

coveralls commented 1 year ago

Coverage Status

Coverage decreased (-0.05%) to 82.107% when pulling 3eef05a0fb934f3cfd9db44671a194ff527bcac2 on LIU-284 into 42ab4fd4fe35f5e8a8f028c1ffb3f0f0ba3dd542 on master.

pritchardn commented 1 year ago

In addition to my previous comment,

This is accomplished by adding a new REST endpoint 'submission_methods' to the engine, translator_rest (formerly lg_web) and (in a separate repo) daliuge-ood. Some limitations/points I need feedback on:

This MR also moves the k8s environment check to a common location and is already adapted to the fastAPI version of the translator.

pritchardn commented 1 year ago

So, with all that out of the way, I've asked for a few reviewers since I need someone to review the usability of this feature, for which I may need help with the UI stuff and someone to review the internal changes, ( @juliancarrivick this may be a good chance to become familiar with this part of the code, hence the request) - happy to change reviewers of course if needed :smile:

awicenec commented 1 year ago

Hi Nicholas,

I've just tried this and have a bit of an issue getting it working at all. With this version my previous settings, which are kept in the browser storage, are all gone and I can't add a new one. When trying it accepts my input, but then does not show anything in the method dropdown box. When I then close and open the settings modal again that new setting is also not there anymore. Moreover the actual deployment does not work either.

Not sure what's happening here. Already tried another browser, but exactly the same effect.

Cheers, Andreas

On Mon, 2022-10-24 at 00:26 -0700, Nicholas Pritchard wrote:

So, with all that out of the way, I've asked for a few reviewers since I need someone to review the usability of this feature, for which I may need help with the UI stuff and someone to review the internal changes, ( @juliancarrivick this may be a good chance to become familiar with this part of the code, hence the request) - happy to change reviewers of course if needed 😄 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

pritchardn commented 1 year ago

Interesting and unfortunate, thanks for trying it out I'll get a fresh install up and running and see if I can see what's happening, I've managed to have it working on my machine, but obviously, that's far from enough.

pritchardn commented 1 year ago

I'll go through these throughout the day, thanks for the comments! :smile:

pritchardn commented 1 year ago

I believe all of the comments have been addressed, feel free to bounce any of the subsequent changes back, better get things right :smile: