-
Notifications
You must be signed in to change notification settings - Fork 6
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
Job output transform #548
base: master
Are you sure you want to change the base?
Job output transform #548
Conversation
Direct Links in job output + makefile + tests
Testing transformer engine added
Remove requirements-trfm Add to main requirements
Why not pandas
Update requirements
Ajout Multi frame Images
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.
There's a few linting issues that can be resolved automatically using the make fix[...]
targets.
Validate as well using make check[...]
targets any other flagged issues that might need some manual adjustments. I didn't comment on all of them.
I did not go in depth into the actual implementation of the transform functions.
I think there is a few tests missing to validate that the actual results from each transformation is valid (eg: pixel values from one format to another are properly rendered or expected text from one format to another is properly structured).
…moving todos and unused import
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.
rescanned quickly the changes, many comments are still open
echo "Binary package manager cairo not found. Attempting to install it."; \ | ||
$(SUDO) apt-get install libpangocairo-1.0-0 \ |
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.
Might need other libraries here. https://cairosvg.org/documentation/
@@ -23,6 +23,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ | |||
g++ \ | |||
git \ | |||
nodejs \ | |||
libpangocairo-1.0-0 \ |
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.
We will need to validate against libraries listed in https://cairosvg.org/documentation/ that are different...
Maybe a custom docker smoke-test or something? Not sure yet.
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.
Since we already have a install-transforms
Makefile target, I think we should consider moving the new dependencies to a dedicated requirements-transform.txt
file to keep things clear.
The corresponding pip install -r requirements-transform.txt
would be added to install-transforms
.
Maybe another PR could explore the optional activation of transform capabilities...
"code": "", | ||
"description": "The requested output Id is not available in the job results.", | ||
"cause": "The output ID is not available", | ||
"error": "", | ||
"value": "" |
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.
Fill the details (output_id
in value
, InvalidIdentifierValue
for code
).
Cause could indicate something more useful, such as The output ID must be one of: {}
with the list retrieved from available IDs in job.results
.
Use "ID" to be consistent across messages.
is_reference = bool(get_any_value(result, key=True, file=True)) | ||
_, output_format = get_job_output_transmission(job, output_id, is_reference) | ||
output_format = accept or output_format or result_media_type | ||
|
||
# To ensure consistency and avoid type mismatches, all output formats are converted to a dictionary. | ||
# This standardization prevents errors when handling cases in `get_job_results_single` | ||
# where some formats are dictionaries and others are different types (e.g., strings or ContentType). | ||
if not isinstance(output_format, dict): | ||
output_format = {"mime_type": output_format} | ||
|
||
return get_job_results_single(job, result, output_id, output_format, headers=headers, settings=settings) |
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.
This is essentially the same as the end of get_job_results_response
:
weaver/weaver/wps_restapi/jobs/utils.py
Lines 758 to 765 in 64db348
res_id = out_vals[0][0] | |
# check accept header | |
req_fmt = (request_headers or {}).get("accept") | |
out_fmt = out_transmissions[res_id][1] | |
out_type = get_field(results[res_id], "mime_type", search_variations=True, default=None) # a voir en debuggant | |
out_select = req_fmt or out_fmt or out_type # (resolution order/precedence) | |
out_fmt = out_select | |
return get_job_results_single(job, out_info, res_id, out_fmt, headers=headers, settings=settings) |
Please define a function like resolve_result_single
or similar, and invoke it in both places.
The output transmission check should already be done by the get_job_results_single
call, with all corresponding logic to convert data/link and transforms.
config.add_cornice_service(sd.process_jobs_service) | ||
config.add_cornice_service(sd.process_job_service) | ||
config.add_cornice_service(sd.process_results_service) | ||
config.add_cornice_service(sd.process_result_value_service) | ||
config.add_cornice_service(sd.process_outputs_service) | ||
config.add_cornice_service(sd.process_output_service) | ||
config.add_cornice_service(sd.process_inputs_service) | ||
config.add_cornice_service(sd.process_exceptions_service) | ||
config.add_cornice_service(sd.process_logs_service) | ||
config.add_cornice_service(sd.process_stats_service) |
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.
sync order with upstream branch to avoid the diff
(fixes
#100 <https://github.com/crim-ca/weaver/issues/100>
_).GET /results/{id}
andGET /outputs/{id}
routes to enable direct access to individualjob result items by ID. This enhancement includes: support alternate representations based on the Accept header.
If an alternate format (e.g., YAML for a JSON source) is requested it will be automatically generated and returned.
Link headers containing all possible output formats, allowing retrieval via query parameters
(e.g., output?f=application/x-yaml). (fixes
#18 <https://github.com/crim-ca/weaver/issues/18>
_).