Closed Ardweaden closed 1 year ago
I'm not actually sure that this should be merged to the openEO API as it seems very specific to Platform and has not had a huge discussion across back-end implementations. As you mentioned, not everything is machine-readable yet and as such it is not really an interoperable API that can be easily be used by our clients etc. Also, it seems not fully integrated with the existing approaches (e.g. integrate with /collections and STAC better). Would it make more sense to have this separate as an extension proposal for openEO or as a separate proprietary API for openEO Platform?
I'm not actually sure that this should be merged to the openEO API as it seems very specific to Platform and has not had a huge discussion across back-end implementations. As you mentioned, not everything is machine-readable yet and as such it is not really an interoperable API that can be easily be used by our clients etc. Also, it seems not fully integrated with the existing approaches (e.g. integrate with /collections and STAC better). Would it make more sense to have this separate as an extension proposal for openEO or as a separate proprietary API for openEO Platform?
@mkadunc @dthiex
Would it make more sense to have this separate as an extension proposal for openEO
This would make sense. Are there any other existing openEO extensions?
Would it make more sense to have this separate as an extension proposal for openEO
This would make sense. Are there any other existing openEO extensions?
I don't see major benefits from this API being proprietary, so I agree with @Ardweaden to make it an open extension of the openEO API.
There's the Federation Extension draft, which you can look at in the draft branch. The main issue with extensions is that you can't extend OpenAPI easily and as such all the OpenAPI extensions are in code block in the Readme.
My thought here is that if you want to use this exact API as proposed (as you likely have it implemented already), you better do a separate release as a community extension. Otherwise, I'd surely go over it and propose some changes that may change the API in a way that you need to do changes. ;-) For example, I'm really wondering why there's everything grouped under /commercial_data while I think you could also merge it into some of the existing endpoints (e.g. /collections). And then just mark collections as "commercial" and then use the STAC order extension (once available). The search endpoint looks like it could just be a STAC API search endpoint? And then there's only /orders left that can be added as new endpoints. That are just the high-level thoughts, I haven't really digged into the details yet.
My thought here is that if you want to use this exact API as proposed (as you likely have it implemented already), you better do a separate release as a community extension.
We haven't implemented it yet so that's not an issue.
Otherwise, I'd surely go over it and propose some changes that may change the API in a way that you need to do changes. ;-) For example, I'm really wondering why there's everything grouped under /commercial_data while I think you could also merge it into some of the existing endpoints (e.g. /collections). And then just mark collections as "commercial" and then use the STAC order extension (once available). The search endpoint looks like it could just be a STAC API search endpoint? And then there's only /orders left that can be added as new endpoints. That are just the high-level thoughts, I haven't really digged into the details yet.
Thanks for great suggestions, we'll look into it and try to align the proposal to match them,
Ah, good to know. Then I'll also invest some time into a deeper review soon.
Hey @Ardweaden, we were talking about this PR recently and I'm about to review it, but I was wondering whether it would be good to set up a call so that we can discuss the overall idea better instead of wrangling through 1000 lines of OpenAPI directly? I can't see your e-mail address here otherwise I'd have directly sent an e-mail.
@Ardweaden fyi: here's the only existing extension, which you can get inspiration from: https://github.com/Open-EO/openeo-api/tree/draft/extensions/federation
@Ardweaden fyi: here's the only existing extension, which you can get inspiration from: https://github.com/Open-EO/openeo-api/tree/draft/extensions/federation
@m-mohr I pushed the first version of README. I decided to revert the previous commits and therefore in the branch for now, so that we can refer to the changes made there if it comes in handy.
Okay, great. I see that you went above and beyond with the order extension ;-) I see there's no OpenAPI document anymore, is that intentional or do you still plan to add one? If this is ready for review, just change the state of this PR accordingly. :-) Thanks.
I see there's no OpenAPI document anymore, is that intentional or do you still plan to add one?
Yes, I think we said in the meeting that we'll first create a README so that we can check understood each other correctly and the overall approach is OK, and then add the OpenAPI file as it's more cumbersome and contains the details.
So I'd appreciate if you read it through and confirm it's ok, and then I'll add the OpenAPI file.
In general this looks pretty good. I've left some comments, some for discussion and some are usually minor fixes.
@Ardweaden Thanks for the updates. I added another round of comments - mostly minor things to align better. Overall this is already looking pretty good 👍
Did a quick review. The part about searching for products seems to be quite independent of commercial data, and has some overlap with property filtering in load_collection. Could that also be a separate extension? (Or part of core?)
That's actually part of STAC API / OGC APIs (/queryables, /search, /collections/:id/items) and it is already mentioned switftly as optional in the openEO API, so doesn't really need a separate extension (I think) as it would just point to an external specification basically. But on the other hand it's simple to do and may clarify it better compared to the short mention in the openEO API. Here it's mentioned explicitly as it's pretty much required for the use case.
Ah yes, so for this extension, the optional support for search becomes a harder requirement? What about the alignment between /queryables and the collection property filtering that was based on summaries?
For ordering you need to provide specific products/items and the only reasonable way to do this is probably search. So yes, this is at least recommended to implement (we don't really require any endpoints in the spec though, you just expose what you offer via GET /
so that's why I don't call it a hard requirement).
What about the alignment between /queryables and the collection property filtering that was based on summaries?
I'm not sure. Are load_collection filtering options and available filters in search always the same? The current draft has openeo:property_filters
and summaries
for this purpose, but I'd also be fine to use /queryables for this if the question can be answered "yes".
@m-mohr I gave it some thought, and can't really come up with an example of a property that is only relevant in one case and not in the other. (load_collection vs search)
All common properties that we see being used often (orbit direction, relative orbit, tile id, cloud cover, instrument mode,...) are all relevant in both cases.
It would be very nice if a user can construct a query for product search, and transfer that to load_collection without requiring much adaptation, although not sure if that would still be possible as we already use openEO style callbacks for property filtering in load_collection right...
The only one that comes directly to mind is likely the collection ID, which doesn't make sense as property filter.
CQL Text is probably something we could allow as alternative, but CQL JSON is as complex as what we have in openEO so there's not a lot of benefit, I'd assume. Especially as I don't know of a lot of existing "clients" that generate CQL in a user-friendly way (and in a way that you can simply copy it).
This is really getting off topic right now, let's continue in another issue if there's further demand.
Edit: Reopened https://github.com/Open-EO/openeo-api/issues/396 for it.
Hey @Ardweaden,
I've worked on this today and made a couple of changes to the OpenAPI document mostly (and removed the endpoint descriptions from the README as they are now fully included in the OpenAPI file). Could you have a look at whether the changes all make sense to you, too? Most notable changes are likely the split of order:id into order:id and id, rename of products to item and moving "item_metadata" links into "links".
I also merged with the draft branch and moved the extension to the correct folder.
If you have any questions on changes etc, please let me know.
From my side this is mostly ready to be merged. If that's the case for you too, please mark this PR as "ready for review".
@m-mohr Thanks a lot.
Looks good on the first glance. I am implementing this spec at the moment, so I'd prefer to continue with merging when that is more or less finished, in case I come across some practical problems that would require changing it.
Merging doesn't mean we can't change anymore, we can always make new PRs to refine it, too. :-)
@Ardweaden The STAC order extension has been released. A couple of minor changes have been made before the release, e.g. the status codes are a bit different now. Can you update the PR to reflect the changes? You can now also add the corresponding entry to stac_extensions for the order extension. Thanks :-)
@Ardweaden The STAC order extension has been released. A couple of minor changes have been made before the release, e.g. the status codes are a bit different now. Can you update the PR to reflect the changes? You can now also add the corresponding entry to stac_extensions for the order extension. Thanks :-)
Thanks, updated. What did you mean with "corresponding entry to stac_extensions"? Should the orders, which are not STAC Items, but use the fields from Order extension specify stac_extensions
?
No, just in actual STAC resources. Looks fine, I think. It was already included.
This PR adds documentation for the commercial data API, which should provide the ability to purchase commercial data. It is mostly based on Sentinel Hub's API, but it's greatly simplified.
This is most certainly not the final version, as there are quite a few things that should be discussed.
Pricing
General information about pricing is only provided in a human-readable way. We could attempt to standardise it, but it might not be so trivial. Actual costs of a specific order are returned in the values of currency upon creating the order.
Search and order parameters
These are again not provided in a computer-readable way as there can be very many different options. However, it would make sense to generalise spatial extent and temporal extent, as they will most likely appear on most collections - however I'm not sure they will be always usable, that's why this proposal does not implement that.
Accessing commercial data
This proposal relies on the setup that VITO already employs, of having information about a "general collection" available at
/collections
, and then a specific identifier needs to be used inload_collection
in order to access the actual purchased data. That's why the orders in this proposal includebyoc_collection_id
, which I think is not really appropriate. The referenced issue is discussed here: https://github.com/openEOPlatform/architecture-docs/issues/4#issuecomment-983451983