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

Changes for closer alignment with OpenEO. #477

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pvretano
Copy link
Contributor

@pvretano pvretano commented Dec 5, 2024

No description provided.

- running
- cancelled
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- cancelled
- canceled

openEO uses "canceled", is it double l in OGC API Processes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines 21 to 23
status:
oneOf:
$ref: "statusCode.yaml"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
status:
oneOf:
$ref: "statusCode.yaml"
status:
oneOf:
$ref: "statusCode.yaml"

Looks invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

- id
- status
- created
- process
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be required or optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted back to the required fields for OAProc and included text in the descriptions based on the value of the "type" property. If the value of "type" is "openeo" then both "created" and "process" MUST be present as per the OpenEO API definition.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Generally, I think we can do better to avoid these if/then conditionals as much as possible :-)

type: string
process:
$ref: process.yaml
type:
Copy link

@m-mohr m-mohr Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process type?

It should probably just describe the encoding of the process property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the description to indicate that if "type" is "openeo" then the "process" property MUST be present and its value shall be a process graph as per the OpenEO API definition. Otherwise if can be an OAProc process description, a WPS process description, a link to either of these or a link to the request that created the job (e.g. an async coverages request).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this condition to be more explicit?

Since we have the allOf, we can make use of it.

allOf:
  - $ref: descriptionType.yaml
  - type: object
    # ... all other fields ...
  - if:
      properties:
        type: 
          const: openeo  # or 'enum' equivalent if prefered
    then:
      required:
        - id
        - type
        - status
        - process
      properties:
        process:
          $ref: "<uri-to-openeo-process-graph>"

Copy link

@m-mohr m-mohr Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether OGC API - Processes should just define the framework and not have direct dependencies to openEO due to the issue that there are multiple versions of the openEO API...

As long as openEO follows the framework, I guess it's fine, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting that anyone that wants to do strong schema validation would actually use the reference openEO schemas rather than depend on OGC API, so it's fine if we don't define field requirements of openEO explicitly here.

However, if we do so, I don't see much point to have the long description either for specific frameworks. Saying in the description that process is required when type: openeo doesn't actually enforce or validate anything.

Copy link

@m-mohr m-mohr Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the spec should not include specifics about openEO. Maybe as an appendix only.

openapi/schemas/processes-core/statusInfo.yaml Outdated Show resolved Hide resolved
type: string
process:
$ref: process.yaml
type:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this condition to be more explicit?

Since we have the allOf, we can make use of it.

allOf:
  - $ref: descriptionType.yaml
  - type: object
    # ... all other fields ...
  - if:
      properties:
        type: 
          const: openeo  # or 'enum' equivalent if prefered
    then:
      required:
        - id
        - type
        - status
        - process
      properties:
        process:
          $ref: "<uri-to-openeo-process-graph>"

Comment on lines +64 to +71
- type: string
enum:
- created
- queued
- running
- canceled
- finished
- error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does openEO have a schema for this?
The definition could be more explicit if we actually pointed to $ref: <uri-openeo-schema#status> or wherever it resides.

Copy link

@m-mohr m-mohr Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do the OGC way with snippets, we only have a monolithic OpenAPI document. In principle you can refer to that though:

"$ref:" "https://raw.githubusercontent.com/Open-EO/openeo-api/refs/tags/1.2.0/openapi.yaml#/components/schemas/batch_job/properties/status"

That reminds me that we should probably version the OpenAPI files on gh-pages with the docs so that it will be

"$ref:" "https://api.openeo.org/1.2.0/openapi.yaml#/components/schemas/batch_job/properties/status"

I hope yaml works, but we could also deploy json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second link would be ideal. It's fine with definition anchors. Either YAML or JSON should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants