Skip to content

Implement support for the Federation Extension #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
8 of 12 tasks
m-mohr opened this issue Jan 7, 2022 · 20 comments · Fixed by #353 or #357
Closed
8 of 12 tasks

Implement support for the Federation Extension #230

m-mohr opened this issue Jan 7, 2022 · 20 comments · Fixed by #353 or #357
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@m-mohr
Copy link
Member

m-mohr commented Jan 7, 2022

See Open-EO/openeo-api#419 and https://github.com/Open-EO/openeo-api/blob/draft/extensions/federation/README.md

An intiial implementation: https://github.com/openEOPlatform/architecture-docs/issues/179#issuecomment-1007453499


In the EOEPCA+ project, we made an evaluation of the federation-related functionality available through the openEO Web Editor.

The following issues have been identified:

  • federation:missing can't be accessed through the corresponding functions for the following endpoints:
    • GET /collections (high prio)
    • GET /processes (high)
    • GET /file_formats (high)
    • GET /process_graphs (high)
    • GET /files (high)
    • GET /jobs (high)
    • GET /jobs/:id/logs (low, missing upstream in Vue Components)
    • GET /services (high)
    • GET /services/:id/logs (low, missing upstream in Vue Components)
  • federation:backends can't be accessed through the corresponding functions for the following endpoints:
    • POST /validation (medium)
    • GET /udf_runtimes per version (low, missing upstream in Vue Components)
    • GET /services/{id} (low, missing upstream in Vue Components)

Related issue for the upstream issues in Vue Components: Open-EO/openeo-vue-components#97

@m-mohr m-mohr added this to the v0.11 milestone Jan 12, 2022
@m-mohr m-mohr self-assigned this Feb 4, 2022
@m-mohr m-mohr modified the milestones: v0.11, v0.12 Apr 7, 2022
@m-mohr m-mohr modified the milestones: v0.12, v0.13 Oct 12, 2022
@m-mohr m-mohr removed their assignment Oct 18, 2022
@m-mohr m-mohr modified the milestones: v0.13, 0.14.0 Dec 8, 2022
@m-mohr m-mohr added enhancement New feature or request and removed Platform labels Mar 15, 2025
@m-mohr
Copy link
Member Author

m-mohr commented Mar 16, 2025

Add a warning icon with the FA icon (<i class="fa-solid fa-triangle-exclamation"></i>) to the following places could work for cases where a missing backend is reported. The button/icon can be hidden if everything is okay.

Image

For jobs, services, files, UDPs a button with icon should work, for collections, processes, file formats it might just be a clickable icon without button style as it might look more pleasing.

The JS client v2.7.0 supports the functionality to retrieve the properties since some months, see also Open-EO/openeo-js-client@fd78bb8 and Open-EO/openeo-js-client@2a14de7

The list of missing backends with a short explanation could be shown in a modal, toast or tooltip.

High prio tasks should be implemented first.
This can be implemented directly to the openeo-web-editor as PRs to the main branch, we can merge this later into the gdc-web-editor repo.

christophfriedrich added a commit that referenced this issue Apr 9, 2025
This can already be used with the version of the js-client on the `federation-as-object` branch there.
@m-mohr m-mohr linked a pull request Apr 10, 2025 that will close this issue
christophfriedrich added a commit that referenced this issue Apr 17, 2025
And one more in the DiscoveryToolbar
@christophfriedrich
Copy link
Collaborator

christophfriedrich commented Apr 22, 2025

Regarding the issue that we don't really like a tooltip on a button, but a modal would also be overkill:

How would you like a "compact" version of the FederationMissingNotice for the panels? Which styling is desired could be set via a boolean compact prop.

Consumes one line of space (two if the availability situation is really bad or the screen particularly small), but would be consistent with the rest and IMO fairly pleasing visually.

Image

@m-mohr
Copy link
Member Author

m-mohr commented Apr 22, 2025

I like that. I'd also expect that usually it's only a single backend, so the text should usually be a bit shorter.

@christophfriedrich
Copy link
Collaborator

Nice, then I'll finish implementing that.

For now I made it as a separate component in the templates of the panels (requiring to wrap everything in a new <div> to ensure only 1 root element), e.g. like this in JobPanel:

<template>
	<div id="JobPanel">
		<FederationMissingNotice ... />
		<DataTable ... >
			<template slot="toolbar">
				...
			</template>
			...
		</DataTable>
	</div>
</template>

Or would you prefer adding a new slot to DataTable? Would then look like this:

<template>
	<DataTable ... >
		<template slot="notice">
			<FederationMissingNotice ... />
		</template>
		<template slot="toolbar">
			...
		</template>
		...
	</DataTable>
</template>

The latter would give the option to switch the places of toolbar and notice, but I prefer the current layout anyway (tools to manipulate the table are directly above it, not split from it by an annoying notice). Semantically I think one could find arguments for either version.

@m-mohr
Copy link
Member Author

m-mohr commented Apr 23, 2025

I think I'd go with the first option, that seems a little more clean.
You'd need to make sure that the scrolling etc still works as before.

@christophfriedrich
Copy link
Collaborator

First real-world displaying of fed:missing information :)) [when connecting to openeo.cloud, i.e. no stump]

Image

m-mohr pushed a commit that referenced this issue Apr 26, 2025
* Pass federation info through to web components (#230)

This can already be used with the version of the js-client on the `federation-as-object` branch there.

* Convert federation info as needed (#230)

See also Open-EO/openeo-js-client#67 (comment)

* Simplify checking for non-federation case

see https://github.com/Open-EO/openeo-web-editor/pull/353/files/5809f11e5917b3d8d465003cf6a25247866c05f1#r2036257667

Co-authored-by: Matthias Mohr <[email protected]>

* Pass federation info to modals and wizards (#230)

And one more in the DiscoveryToolbar

* Fix typo

* Show FedMissingNotice in user content panels (#230)

* Adapt CSS name, fix getDefaultState, cleanup

see Open-EO/openeo-vue-components#108
and #353 (comment)
and #353 (comment) respectively

* Upgrade dependencies
@m-mohr m-mohr reopened this Apr 26, 2025
@christophfriedrich
Copy link
Collaborator

Regarding POST /validation, do you have wishes on how the presence/non-presence (and possibly content) of federation:backends should be communicated in the UI?

Currently the only idea I have would be in this part of the code, adapting the message of the toast in the else branch and adding one with Utils.error() to the if branch:

let errors = await this.connection.validateProcess(this.process);
if (errors.length > 0) {
errors.forEach(error => error.level = 'error');
this.broadcast('viewLogs', errors, 'Validation Result', 'fa-tasks');
return false;
}
else {
Utils.ok(this, "The process is valid");
return true;
}

@m-mohr
Copy link
Member Author

m-mohr commented Apr 26, 2025

I had to quickly think what the property probably means for each case:

  • error: I've checked the given backends and the following errors occured.
  • ok: It is runnable on the given backends (there might be others, but status is either errored/unsupported/unavailable/...)

If logs are shown, we could prepend a log entry at the beginning with the severity of info.
For the other case, I'd probably show a second toast (Utils.info, consistent with the logs) that explains that the process can be executed on backends X, Y and Z.

Would you agree?

@christophfriedrich
Copy link
Collaborator

christophfriedrich commented Apr 28, 2025

What it means in each case is stated very explicitly here: https://github.com/Open-EO/openeo-api/blob/draft/extensions/federation/README.md#validation (however I still had a question, see Open-EO/openeo-api#563) Your summary aligns with that, yes.

I also thought about adding a log entry, but didn't want to interfere with the raw data. And as we need a toast in the "OK" case anyway, I thought it would only be consistent for the "error" case to show the same info in the same way too.

I'd probably show a second toast

What's the benefit of two toasts instead of squashing the infos into one? Also idk what you mean with "Utils.info, consistent with the logs".

Have a look at the commit I just pushed to see how I mean my solution. (It's preliminary, I'm open for suggestions of changes.)

@m-mohr
Copy link
Member Author

m-mohr commented Apr 28, 2025

Toasts are rather short-lived and also have restricted space, thus toast are pretty much the last resort in this case.
I'm not sure how it works with your current proposal, but the intention of the second toast was just to ensure everything will be shown. Interfering with the logs is fine I think. It's more persistent, but opening a new logfile just for reporting the federation details seems over the top for the success case. Generally, the okay case is less important as in this case you can just submit the workflow and it will run and as such the short-lived toast is fine, one could even argue we don't need to show the information in this case. In the error case you may need the information for support requests or so, so I think having it in the logs and not in the toast is the better choice.

The hint at utils.info is just about the "log level". We should use for a informational toast/log that shows the federation information the log level "info" in both cases.

@christophfriedrich
Copy link
Collaborator

You've got good points why the info should (also) be in the logs. Convinced.
And ah okay, you mean the log level, that makes sense.

I'll add that to the code and push something new.

@christophfriedrich
Copy link
Collaborator

Looks like this now:

Image

Of course that might get a little long for a toast if there are more backends:

Image

But looks good when expanded in the logs:

Image

(Note the InsertedByWebEditor, I had to give it some ID so I chose that)

-> Keep the current message for the logs and shorten it for the toast?

Options:

  • The process is invalid, as checked by 3 back-ends of the federation (see logs for details)
  • The process is invalid, as checked by 3 back-ends of the federation
  • The process is invalid (see logs for details)
  • The process is invalid

@christophfriedrich
Copy link
Collaborator

For the positive case, we don't want to open anything except the toast, so if we want to show the information at all, we have no choice than to put it all into that toast text:

Image

So if there's more than 1 backend that can run it, chances are pretty high that it's not gonna get displayed:

Image

This could be fixed by not replacing the IDs with the titles, as the IDs are usually much shorter, but at the cost of uglier UI that is also somewhat inconsistent as we replace with the titles everywhere else...

Image

These shortening options would hold back information from the user:

  • The process is valid, as checked by 3 back-ends of the federation
  • The process is valid

@m-mohr
Copy link
Member Author

m-mohr commented Apr 30, 2025

For the error case, I agree. We should keep it short in the toast and give details in the logs.
So the toast should likely just say:

The process is invalid (see logs for details)

As discussed before, I think the federation details in the positive case are not so important.
I'd go for:

The process is valid

@christophfriedrich
Copy link
Collaborator

So the cases listed in the spec now look like this respectively:

  • Endpoint returns without errors:
    • federation:backends is included in the response: The listed back-ends support the workflow (either partially if splitting is supported, or in full).
      Image
    • federation:backends is not included in the response: At least one of the back-ends support the workflow.
      Image
  • Endpoint returns errors:
    • federation:backends is included in the response: The listed back-ends were checked and none of the back-ends can run the workflow as is (neither splitted if supported, nor in full).
      Image
    • federation:backends is not included in the response: the workflow could not be validated successfully by any of the back-ends or the federation component itself.
      Image

I kept the comparatively long message in the last case because its situation is not (necessarily) the same as "The process is invalid". If you want to rephrase it I'm open for suggestions, for now I kept it like in the spec so that everything aligns/I don't introduce ambiguities/misalignments.

Another option that came to my mind is making the toast clickable (like it happens already e.g. with HTTP errors) and put the details there (e.g. in the positive case), so that this information would be principally available.

@m-mohr
Copy link
Member Author

m-mohr commented Apr 30, 2025

Let's focus on the basics, conveying the most important information to users.

First question: Does this all only apply if there's a federation?
federation:backends missing without further conditions would also apply if it's just a backend without being part of a federation.

  • Endpoint returns without errors:
    • federation:backends is included in the response: Good.
    • federation:backends is not included in the response: Good.
  • Endpoint returns errors:
    • federation:backends is included in the response: Good.
    • federation:backends is not included in the response: In this case it doesn't really matter. I think the toast is good enough and we don't need to add it to the logs.

@christophfriedrich
Copy link
Collaborator

First question: Does this all only apply if there's a federation?

Yes, I implemented a check isFederated:

let isFederated = Utils.size(this.connection.capabilities().toJSON().federation) > 0;

Which is then checked before all the other ifs:

if(isFederated) {
if(Array.isArray(errors['federation:backends'])) {

(The check uses the same logic I found in vue-components Capabilities.vue -- we could also make it a queriable attribute/method on the Connection object, but for now I didn't bother, for focusing reasons ;))

In the non-federated case, it's "The process is valid/invalid" as before.


Good. Good. Good.

Nice

I think the toast is good enough and we don't need to add it to the logs.

IMO it doesn't hurt either and as it's a long message it might be nice to be able to read it again there. Also makes behaviour in the error case a bit more consistent.

So overall I would leave it as it is right now and we could merge this.

@m-mohr
Copy link
Member Author

m-mohr commented May 4, 2025

Okay, this is an issue though. So there's nothing to merge yet.

@christophfriedrich
Copy link
Collaborator

True, I'll open the PR in a sec

@m-mohr
Copy link
Member Author

m-mohr commented May 4, 2025

Closing in favor of #358.

@m-mohr m-mohr closed this as completed May 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants