-
-
Notifications
You must be signed in to change notification settings - Fork 253
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
ogc api processes subscriber #1313
Changes from all commits
b6332e1
9637849
d1c208d
b86e7ed
cf4da59
0ded1c5
89fe615
a20d10b
89f8c4d
1506e38
e01791d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,8 @@ | |
from typing import Any, Dict, Tuple, Optional, OrderedDict | ||
import uuid | ||
|
||
import requests | ||
|
||
from pygeoapi.plugin import load_plugin | ||
from pygeoapi.process.base import ( | ||
BaseProcessor, | ||
|
@@ -50,6 +52,7 @@ | |
JobStatus, | ||
ProcessExecutionMode, | ||
RequestedProcessExecutionMode, | ||
Subscriber, | ||
) | ||
|
||
LOGGER = logging.getLogger(__name__) | ||
|
@@ -70,6 +73,7 @@ def __init__(self, manager_def: dict): | |
|
||
self.name = manager_def['name'] | ||
self.is_async = False | ||
self.supports_subscribing = False | ||
self.connection = manager_def.get('connection') | ||
self.output_dir = manager_def.get('output_dir') | ||
|
||
|
@@ -85,7 +89,7 @@ def __init__(self, manager_def: dict): | |
for id_, process_conf in manager_def.get('processes', {}).items(): | ||
self.processes[id_] = dict(process_conf) | ||
|
||
def get_processor(self, process_id: str) -> Optional[BaseProcessor]: | ||
def get_processor(self, process_id: str) -> BaseProcessor: | ||
"""Instantiate a processor. | ||
|
||
:param process_id: Identifier of the process | ||
|
@@ -178,7 +182,9 @@ def delete_job(self, job_id: str) -> bool: | |
raise JobNotFoundError() | ||
|
||
def _execute_handler_async(self, p: BaseProcessor, job_id: str, | ||
data_dict: dict) -> Tuple[str, None, JobStatus]: | ||
data_dict: dict, | ||
subscriber: Optional[Subscriber] = None, | ||
) -> Tuple[str, None, JobStatus]: | ||
""" | ||
This private execution handler executes a process in a background | ||
thread using `multiprocessing.dummy` | ||
|
@@ -194,13 +200,15 @@ def _execute_handler_async(self, p: BaseProcessor, job_id: str, | |
""" | ||
_process = dummy.Process( | ||
target=self._execute_handler_sync, | ||
args=(p, job_id, data_dict) | ||
args=(p, job_id, data_dict, subscriber) | ||
) | ||
_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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No docstring for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll push a fix. |
||
) -> Tuple[str, Any, JobStatus]: | ||
""" | ||
Synchronous execution handler | ||
|
||
|
@@ -233,6 +241,7 @@ def _execute_handler_sync(self, p: BaseProcessor, job_id: str, | |
} | ||
|
||
self.add_job(job_metadata) | ||
self._send_in_progress_notification(subscriber) | ||
|
||
try: | ||
if self.output_dir is not None: | ||
|
@@ -276,6 +285,7 @@ def _execute_handler_sync(self, p: BaseProcessor, job_id: str, | |
} | ||
|
||
self.update_job(job_id, job_update_metadata) | ||
self._send_success_notification(subscriber, outputs=outputs) | ||
|
||
except Exception as err: | ||
# TODO assess correct exception type and description to help users | ||
|
@@ -308,13 +318,16 @@ def _execute_handler_sync(self, p: BaseProcessor, job_id: str, | |
|
||
self.update_job(job_id, job_metadata) | ||
|
||
self._send_failed_notification(subscriber) | ||
|
||
return jfmt, outputs, current_status | ||
|
||
def execute_process( | ||
self, | ||
process_id: str, | ||
data_dict: dict, | ||
execution_mode: Optional[RequestedProcessExecutionMode] = None | ||
execution_mode: Optional[RequestedProcessExecutionMode] = None, | ||
subscriber: Optional[Subscriber] = None, | ||
totycro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) -> Tuple[str, Any, JobStatus, Optional[Dict[str, str]]]: | ||
""" | ||
Default process execution handler | ||
|
@@ -323,6 +336,7 @@ def execute_process( | |
:param data_dict: `dict` of data parameters | ||
:param execution_mode: `str` optionally specifying sync or async | ||
processing. | ||
:param subscriber: `Subscriber` optionally specifying callback urls | ||
|
||
:raises UnknownProcessError: if the input process_id does not | ||
correspond to a known process | ||
|
@@ -367,9 +381,39 @@ def execute_process( | |
response_headers = None | ||
# TODO: handler's response could also be allowed to include more HTTP | ||
# headers | ||
mime_type, outputs, status = handler(processor, job_id, data_dict) | ||
mime_type, outputs, status = handler( | ||
processor, | ||
job_id, | ||
data_dict, | ||
# only pass subscriber if supported, otherwise this breaks existing | ||
# managers | ||
**({'subscriber': subscriber} if self.supports_subscribing else {}) | ||
totycro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
return job_id, mime_type, outputs, status, response_headers | ||
|
||
def _send_in_progress_notification(self, subscriber: Optional[Subscriber]): | ||
if subscriber and subscriber.in_progress_uri: | ||
response = requests.post(subscriber.in_progress_uri, json={}) | ||
LOGGER.debug( | ||
f'In progress notification response: {response.status_code}' | ||
) | ||
|
||
def _send_success_notification( | ||
self, subscriber: Optional[Subscriber], outputs: Any | ||
): | ||
if subscriber: | ||
response = requests.post(subscriber.success_uri, json=outputs) | ||
LOGGER.debug( | ||
f'Success notification response: {response.status_code}' | ||
) | ||
|
||
def _send_failed_notification(self, subscriber: Optional[Subscriber]): | ||
if subscriber and subscriber.failed_uri: | ||
response = requests.post(subscriber.failed_uri, json={}) | ||
LOGGER.debug( | ||
f'Failed notification response: {response.status_code}' | ||
) | ||
|
||
def __repr__(self): | ||
return f'<BaseManager> {self.name}' | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -591,6 +591,17 @@ class JobStatus(Enum): | |
dismissed = 'dismissed' | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Subscriber: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pretty much agree, I prefer a more explicit style here. |
||
"""Store subscriber urls as defined in: | ||
|
||
https://schemas.opengis.net/ogcapi/processes/part1/1.0/openapi/schemas/subscriber.yaml # noqa | ||
""" | ||
success_uri: str | ||
in_progress_uri: Optional[str] | ||
failed_uri: Optional[str] | ||
|
||
|
||
def read_data(path: Union[Path, str]) -> Union[bytes, str]: | ||
""" | ||
helper function to read data (file or network) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)