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
fmigneault
left a comment
There was a problem hiding this comment.
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
fmigneault
left a comment
There was a problem hiding this comment.
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.
Might need other libraries here. https://cairosvg.org/documentation/
| g++ \ | ||
| git \ | ||
| nodejs \ | ||
| libpangocairo-1.0-0 \ |
There was a problem hiding this comment.
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.
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.
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.
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
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.
sync order with upstream branch to avoid the diff
There was a problem hiding this comment.
A better name for this file would be handlers.py, to avoid the many "transform" when doing from weaver.transform.transform import Transform. If CONVERSION_DICT and EXCLUDED_TYPES were defined in from weaver.transform.const, everything else left in this file is to define the various "handlers".
(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>_).