Skip to content
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

list secondary service functionality #78

Closed
dthiex opened this issue Oct 17, 2022 · 16 comments
Closed

list secondary service functionality #78

dthiex opened this issue Oct 17, 2022 · 16 comments

Comments

@dthiex
Copy link

dthiex commented Oct 17, 2022

Is this something that could be added to the aggregator?

Connected to the on-demand viewer SAP02 task as mentioned here: Open-EO/openeo-web-editor#277 (comment)

Open questions:

How to deal with situation where the functionality is not supported by a backend?

I think for the start we could just throw a warning (we will run more and more in cases where certain specific things are not working the same on all backends) and document it on docs.openeo.cloud

@soxofaan
Copy link
Member

adding secondary services support to the aggregator shouldn't be too much work as it is mainly proxying/aggregating secondary service info from upstream backends.

We could start with the bare basics: list services and create a new service.
Other parts of the API (e.g. modifying, deleting, getting logs) could be for a follow up iteration

@dthiex
Copy link
Author

dthiex commented Oct 18, 2022

We could start with the bare basics: list services and create a new service.

Sounds good to me and should be enough to support the on-demand preview functionality as implemented by Matthias.

JohanKJSchreurs added a commit that referenced this issue Nov 14, 2022
…ot supposed to be added to AggregatorBackendImplementation
JohanKJSchreurs added a commit that referenced this issue Nov 15, 2022
@jdries
Copy link
Contributor

jdries commented Nov 22, 2022

PR ready and reviewed.

@jdries jdries closed this as completed Nov 22, 2022
@soxofaan
Copy link
Member

@dthiex the initial implementation is now available, e.g. https://openeocloud-dev.vito.be/openeo/1.0/service_types

Feel free to report issues here. (We couldn't test yet with real upstream backend, because it is a less widely covered feature)

@dthiex
Copy link
Author

dthiex commented Nov 25, 2022

@soxofaan Cool! Overall it seems to work and I see services listed. I tested and already noticed a few things:

  1. creation of xyz service with federated collection fails as the aggregator tries to create it on the vito backend which doesn't support xzy service. I think at the current stage where we have xyz service support only on the SH backend and wmts service support only on the vito backend the aggregator can simply decide based on that where to send the service request to (linked to point 4)

  2. get request to "https://openeocloud-dev.vito.be/openeo/1.0/services" returns "Server error: KeyError('Missing ServiceMetadata fields: attributes, process.')"

  3. Show on Map functionality in the editor (tab: web services) doesn't work anymore as the button tries to create a WMTS service on the Vito backend which seems to be not work at the moment ("Failed to create secondary service on backend 'vito': OpenEoApiError('[500] Internal: Server error: TypeError("'JavaPackage' object is not callable") (ref: r-05d6b28fbcfc49f98a4e7a32fe52370b)')"), simple_pg_secondary_service.txt. I suggest we check what's the status on your wmts service and maybe exclude it from being listed here if it's not working currently?

  4. It seems that to determine where to send service (create service) the aggregator looks at on which backend the collection is. Does that make sense or wouldn't it be better to check which backend supports with service type and decide based on the selected service type? Like that it would be up to the backend or the aggregator to report an error if it can't support the collection.

@soxofaan
Copy link
Member

Thanks for the feedback!

The problem with dispatching to correct upstream back-end (points 1 and 4) is indeed a known issue (listed todo under #78 (comment)). I hope you can workaround that for now by using a SentinelHub-specific collection? I also started discussion at #83 to allow about quick fix for this.

regarding points 2 and 3: the VITO implmentation indeed grew defunct due to lack of interest and we decided to just disable it for now.

@soxofaan
Copy link
Member

with 9730e88 I also hardcoded "sentinelhub" as upstream back-end for all secondary service creation (for now), so that should also simplify your testing

@dthiex
Copy link
Author

dthiex commented Dec 1, 2022

Those two things indeed improved testing. 1, 3 and 4 work now (are not relevant anymore and our backend gives an understandable error message if you create a service with a collection we don't support).

2 is however still present and I think the aggregator still requires something that is not needed (at lest the response from our backend to the /services endpoint looks okay.

soxofaan added a commit to Open-EO/openeo-python-driver that referenced this issue Dec 1, 2022
soxofaan added a commit that referenced this issue Dec 1, 2022
…kend id

- Use updated ServiceMetadata with more optional fields
- add backend id prefix
- move related tests to test_views.py
@soxofaan
Copy link
Member

soxofaan commented Dec 2, 2022

@dthiex 2 should now also be fixed. I think I kind of managed to create a service now through the aggregator on sentinelhub (id "sentinelhub-eb9fa426-68f1-4a78-92c9-1c573966f63c")

@soxofaan
Copy link
Member

soxofaan commented Dec 7, 2022

FYI I closed the todo notes from #78 (comment) because these task got their own tickets (e.g. #83 , #84 and #85)

@soxofaan
Copy link
Member

soxofaan commented Dec 7, 2022

I think we can close #78 now and leave the work to other tickets mentioned above.

Feel free to reopen or create new issues for follow up

@soxofaan soxofaan closed this as completed Dec 7, 2022
@dthiex
Copy link
Author

dthiex commented Dec 13, 2022

@soxofaan Thanks Stefaan. I came around to do another test and 2 is indeed fixed now so we can indeed follow up in the other issues.

soxofaan added a commit that referenced this issue Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants