ogc api processes subscriber #1313

totycro · 2023-07-05T14:06:25Z

Overview

Add support for OGC API Processes Subscriber

The subscription URLs are passed to the manager, which
then has to call them appropriately.

By default, managers have the attribute supports_subscribing
set to False in order to not break the API for these. The
subscriptions are only passed to if this is set to True

Related Issue / Discussion

#1285

Additional Information

Contributions and Licensing

(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)

I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution.
I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines

ricardogsilva

I'm not a core committer, so take these with a grain of salt while you wait for a review from maintainers 😄

Nice work adding this missing functionality, I just wish we could make it a bit more generic and enforce its support for all managers.

pygeoapi/util.py

ricardogsilva · 2023-07-05T15:00:56Z

pygeoapi/util.py

@@ -571,6 +571,17 @@ class JobStatus(Enum):
    dismissed = 'dismissed'


+@dataclass(frozen=True)
+class Subscriber:


While I do like the idea of using a dataclass, it seems a bit inconsistent that this introduces a difference on how the various parts of an execute request are parsed, as the main execution inputs are not parsed into a typed data structure.

Ideally we would parse the full request into a typed instance (using pydantic ❤️ ) and pass that to the inner code base.

This is not to say I am against this change (quite the contrary), just food for thought for @tomkralidis 😄

I pretty much agree, I prefer a more explicit style here.

ricardogsilva · 2023-07-05T15:06:38Z

pygeoapi/api.py

@@ -168,6 +168,7 @@
        'http://www.opengis.net/spec/ogcapi-processes-1/1.0/conf/core',
        'http://www.opengis.net/spec/ogcapi-processes-1/1.0/conf/json',
        'http://www.opengis.net/spec/ogcapi-processes-1/1.0/conf/oas30'
+        'http://www.opengis.net/spec/ogcapi-processes-1/1.0/conf/callback',


Given that you introduce the subscriber functionality as an optional property of the manager, then I think we cannot assume it by default.

We would need to add this at runtime, by checking if the manager supports it.

What if we simply assume that subscriber is to be always supported? (More on this below)

ricardogsilva · 2023-07-05T15:08:37Z

pygeoapi/process/manager/base.py

@@ -232,6 +239,11 @@ def _execute_handler_sync(self, p: BaseProcessor, job_id: str,
        }

        self.add_job(job_metadata)
+        if subscriber and subscriber.inProgressUri:
+            response = requests.post(subscriber.inProgressUri, json={})


Given my comment above about assuming pygeoapi would want to always support the subscriber feature, how about putting subscribe-related code into a method of the BaseManager class? - this method could then be reused by managers, lowering the cost of supporting this.

pygeoapi/process/manager/base.py

totycro · 2023-07-06T12:40:23Z

@ricardogsilva Thanks a lot for the quick feedback!

About the question whether subscriber support should be mandatory in the manager:

My reasoning was that currently existing managers wouldn't support it, but I'm not sure how many of those are really out there.

I'm currently maintaining the Kubernetes Job Manager here, which passes the execution to a Kubernetes job. So the manager itself doesn't really know when the job finishes and can't easily send success/failed notifications. I'm going to try to get this to work somehow, but in other cases maintainers might not want to add support for this.

However if there are no such managers out there, we can simply make it mandatory and remove the supports_subscribing logic.

An exception is raised in case of error, so it can't ever return None

The subscription URLs are passed to the manager, which then has to call them appropriately. By default, managers have the attribute `supports_subscribing` set to `False` in order to not break the API for these. The subscriptions are only passed to if this is set to `True`

https://docs.ogc.org/is/18-062r2/18-062r2.html#toc67

@ricardogsilva

It's mandatory in the standard. Thx @ricardogsilva !

@ricardogsilva

Thx @ricardogsilva !

@ricardogsilva

Thanks @ricardogsilva !

@ricardogsilva

This increases reusability by other managers Thanks @ricardogsilva !

francescoingv · 2024-03-20T08:44:34Z

pygeoapi/process/manager/base.py

        )
        _process.start()
        return 'application/json', None, JobStatus.accepted

    def _execute_handler_sync(self, p: BaseProcessor, job_id: str,
-                              data_dict: dict) -> Tuple[str, Any, JobStatus]:
+                              data_dict: dict,
+                              subscriber: Optional[Subscriber] = None,


No docstring for subscriber parameter.

I don't have the permissions to push the repo directly and not sure if this warrants a PR, would you mind adding something like this line to the sync and async execute methods?
:param subscriber: optional `Subscriber` specifying callback URLs

I'll push a fix.

@ricardogsilva

* Exclude None from `get_processor` return type annotation An exception is raised in case of error, so it can't ever return None * Add support for OGC API Processes Subscriber The subscription URLs are passed to the manager, which then has to call them appropriately. By default, managers have the attribute `supports_subscribing` set to `False` in order to not break the API for these. The subscriptions are only passed to if this is set to `True` * Add ogc api callback class to conformance https://docs.ogc.org/is/18-062r2/18-062r2.html#toc67 * Make successUri mandatory in subscriber It's mandatory in the standard. Thx @ricardogsilva ! * Use snake case in python for fields which are camel case in the api Thx @ricardogsilva ! * Add subscriber to method docstring * Provide default value for subscriber for managers not supporting it Thanks @ricardogsilva ! * Factor out notification call into methods This increases reusability by other managers Thanks @ricardogsilva ! * Add an example call for a process subscriber * Change test urls to valid urls * Third party imports in own block

* Move api to subdirectory * Move processes api to own file * Adapt processes view methods * Move openapi definition to processes api * Use processes api in flask * Linter * Fix import issues * Allow calling refactored views from starlette * Allow calling refactored views from django * Linter * Move edr api to own file * Adapt edr api to new style * Fix typo in django views * Move maps api to own file * Adapt maps api to new style * Move edr openapi to edr api file * Move maps openapi to maps api file * Move stac views to own file * Refactor stac views to new file * Move stac openapi to stac api file * Move tiles api to own file * Adapt tiles api to new style * Also move tilematrixset to tiles api * Adapt tilesetmatrix views to new style NOTE: I had to remove one tilematrixsets test because it tested that an invalid format would produce an error. This now happens by default for all views, but the actual code is outside of the endpoint function. * update features, records, coverages * update release version * switch back to dev * backport of #1313 * backport of #1313 fix * backport of #1585 * Flask: sanitize OGC schema pathing (#1593) * update release version * switch back to dev * backport of #1596 * Port test_gzip_csv test Note that apply_gzip is now called by the web framework adapters, so to test it in general, we have to call it in the test manually * Add empty conformance class list to stac api * Fix queryables call in starlette * fix ref * Unify request validity checking The default case is handled by the web framework adapters. If custom format handling is required, the check in the adapter must be skipped. * Fix imports in django views * backport #1598 * Remove test about format handling in endpoint This is now handled outside of the endpoint function * add docstring to base process manager (#1603) * backport of #1601 * Port api ogr tests to new style * Move processes tests to own file * Run api tests from new dir in CI * Move edr tests to own file * Move maps tests to own file * Move tiles tests to own file * Actually hide hidden layers in openapi * 1600 allow providing default value in config (#1604) * move coverages tests to own file * move itemtypes to own file, move core into init test * fix OpenAPI output * update tests * add missing descriptions to OpenAPI admin responses * update tests * fix tests autodiscovery * remove unused logging in tests * address PR comments * test with xarray 2024.2.0 * remove unneeded file * safeguard xarray error * unpin xarray * fix OpenAPI generation * fix schema endpoint in Flask and Starlette * Safely serialize configuration JSON (#1605) * Safely serialize configuration JSON Co-Authored-By: Tom Kralidis <tomkralidis@gmail.com> * Revert "Safely serialize configuration JSON" This reverts commit 36feb06. * Add test for datetime with Admin API * Safely serialize configuration JSON --------- Co-authored-by: Tom Kralidis <tomkralidis@gmail.com> * backport #1611 * Also fix schema endpoint for django Fix is analogous to e72d4ba * address additional PR comments --------- Co-authored-by: Tom Kralidis <tomkralidis@gmail.com> Co-authored-by: Angelos Tzotsos <gcpp.kalxas@gmail.com> Co-authored-by: Ricardo Garcia Silva <ricardo.garcia.silva@gmail.com> Co-authored-by: Benjamin Webb <40066515+webb-ben@users.noreply.github.com>

ricardogsilva suggested changes Jul 5, 2023

View reviewed changes

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from d61f7f9 to a282ebe Compare July 6, 2023 12:45

totycro mentioned this pull request Dec 14, 2023

Parameter 'outputs' from request object is not propagated to the manager execute_process and then to processor execute #1420

Closed

This was referenced Dec 18, 2023

ogc api processes - outputs #1420 #1435

Closed

Add support for parameter Outputs of OGC API Processes #1448

Closed

tomkralidis self-requested a review January 3, 2024 13:43

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from 5a3ae79 to 51ef768 Compare January 3, 2024 14:19

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from 51ef768 to 5f061ef Compare February 6, 2024 13:45

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from 5f061ef to 39e3603 Compare February 28, 2024 08:54

tomkralidis mentioned this pull request Mar 6, 2024

Proposal to split api.py into different files #1405

Merged

2 tasks

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from 39e3603 to feed719 Compare March 6, 2024 14:45

totycro added 9 commits March 6, 2024 16:40

Exclude None from get_processor return type annotation

b6332e1

An exception is raised in case of error, so it can't ever return None

Add ogc api callback class to conformance

d1c208d

https://docs.ogc.org/is/18-062r2/18-062r2.html#toc67

Make successUri mandatory in subscriber

b86e7ed

It's mandatory in the standard. Thx @ricardogsilva !

Use snake case in python for fields which are camel case in the api

cf4da59

Thx @ricardogsilva !

Add subscriber to method docstring

0ded1c5

Provide default value for subscriber for managers not supporting it

89fe615

Thanks @ricardogsilva !

Factor out notification call into methods

a20d10b

This increases reusability by other managers Thanks @ricardogsilva !

Add an example call for a process subscriber

89f8c4d

totycro force-pushed the 1285-ogc-api-processes-subscriber branch from ceece82 to 89f8c4d Compare March 6, 2024 15:41

StefanBrand mentioned this pull request Mar 11, 2024

Process execution request parsing not aligned with OGC API - Processes #1285

Open

totycro added 2 commits March 11, 2024 12:00

Change test urls to valid urls

1506e38

Third party imports in own block

e01791d

tomkralidis merged commit 94ae782 into geopython:master Mar 11, 2024
4 checks passed

tomkralidis deleted the 1285-ogc-api-processes-subscriber branch March 11, 2024 11:16

tomkralidis added a commit to totycro/pygeoapi that referenced this pull request Mar 17, 2024

backport of geopython#1313

7842a00

tomkralidis added a commit to totycro/pygeoapi that referenced this pull request Mar 17, 2024

backport of geopython#1313 fix

0ca2dc9

francescoingv reviewed Mar 20, 2024

View reviewed changes

tomkralidis mentioned this pull request Mar 20, 2024

add docstring to base process manager #1603

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ogc api processes subscriber #1313

ogc api processes subscriber #1313

totycro commented Jul 5, 2023

ricardogsilva left a comment

ricardogsilva Jul 5, 2023

totycro Jul 6, 2023

ricardogsilva Jul 5, 2023

ricardogsilva Jul 5, 2023

totycro commented Jul 6, 2023

francescoingv Mar 20, 2024 •

edited

totycro Mar 20, 2024

tomkralidis Mar 20, 2024

ogc api processes subscriber #1313

ogc api processes subscriber #1313

Conversation

totycro commented Jul 5, 2023

Overview

Related Issue / Discussion

Additional Information

Contributions and Licensing

ricardogsilva left a comment

Choose a reason for hiding this comment

ricardogsilva Jul 5, 2023

Choose a reason for hiding this comment

totycro Jul 6, 2023

Choose a reason for hiding this comment

ricardogsilva Jul 5, 2023

Choose a reason for hiding this comment

ricardogsilva Jul 5, 2023

Choose a reason for hiding this comment

totycro commented Jul 6, 2023

francescoingv Mar 20, 2024 • edited

Choose a reason for hiding this comment

totycro Mar 20, 2024

Choose a reason for hiding this comment

tomkralidis Mar 20, 2024

Choose a reason for hiding this comment

francescoingv Mar 20, 2024 •

edited