-
Notifications
You must be signed in to change notification settings - Fork 0
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
update translator service to bclconvert manager #299
Conversation
Morning Ray. Great start. Could you do me favour; can you create a github issue/ticket with the same description - and, label "pipeline" "refactor". Then, pls link it to; this PR > Development > Link issue from this repository (at the right panel) Thank so much. |
Also reformatted python file and added example execution
…ert-manager Thanks @alexiswl , will merge and test it later
Hi @reisingerf @victorskl @alexiswl ;
(Non-SUCCEEDED Event without payload) {
"portalRunId": '20xxxxxxxxxx',
"executionId": "valid_payload_id",
"timestamp": "2024-00-25T00:07:00Z",
"status": "SUCCEEDED",
"workflowName": "BclConvert",
"workflowVersion": "4.2.7",
"workflowRunName": "123456_A1234_0000_TestingPattern",
} (SUCCEEDED Event) {
"portalRunId": '20xxxxxxxxxx',
"executionId": "valid_payload_id",
"timestamp": "2024-00-25T00:07:00Z",
"status": "SUCCEEDED",
"workflowName": "BclConvert",
"workflowVersion": "4.2.7",
"workflowRunName": "123456_A1234_0000_TestingPattern",
"payload": {
"refId": None,
"version": "0.1.0",
"data": {
"projectId": "valid_project_id",
"analysisId": "valid_payload_id",
"userReference": "123456_A1234_0000_TestingPattern",
"timeCreated": "2024-01-01T00:11:35Z",
"timeModified": "2024-01-01T01:24:29Z",
"pipelineId": "valid_pipeline_id",
"pipelineCode": "BclConvert v0_0_0",
"pipelineDescription": "BclConvert pipeline.",
"pipelineUrn": "urn:ilmn:ica:pipeline:123456-abcd-efghi-1234-acdefg1234a#BclConvert_v0_0_0"
"instrumentRunId": "12345_A12345_1234_ABCDE12345",
"basespaceRunId": "1234567",
"samplesheetB64gz": "H4sIAFGBVWYC/9VaUW+jOBD+Kyvu9VqBgST0njhWh046..."
}
}
} |
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.
LGTM
Minor comments.
'id_type': {'S': 'db_uuid'}, | ||
'analysis_id': {'S': internal_ica_event.detail.payload.get('data', '').get("analysisId", '')}, | ||
'analysis_status': {'S': internal_ica_event.detail.status}, | ||
# 'SUCCEEDED', 'FAILED', 'INPROGRESS', 'ABORTED', 'UNKNOWN |
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.
Where does "UNKNOWN" come from?
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 may come from old draft, already deleted.
"portalRunId": '20xxxxxxxxxx', | ||
"executionId": "valid_payload_id", | ||
"timestamp": "2024-00-25T00:07:00Z", | ||
"status": "SUCCEEDED", |
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 should probably not be "SUCCEEDED".
The code will use whatever was the status from the input event, right?
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.
fixed
"workflowVersion": "4.2.7", | ||
workflowRunName: "123456_A1234_0000_TestingPattern", | ||
"payload": { | ||
"refId": None, |
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.
"refId" is no longer needed. (I think the code is already updated)
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.
refId deleted as not required
# Convert from entity module to internal event details | ||
def get_event_details(event) -> WorkflowRunStateChange: | ||
# Extract relevant fields from the event payload | ||
analysis_status = event.get("eventParameters", {}).get("analysisStatus", '') |
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.
Would "UNSPECIFIED" be a better default?
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.
sure, added
|
||
event_name, version = parse_event_code(pipeline.get("code", '')) | ||
|
||
if analysis_status != "SUCCEEDED": |
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.
An empty payload for all non-SUCCEEDED events is fine for now.
In the future we may want to handle all known statuses, for example add an error message payload for FAILED events. Happy to put that on the backlog though.
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.
sure, new backlog added here
I think for non-final status "Requested", "Queued", "Initializing", "Preparing Inputs", "In Progress", "Generating outputs", "Aborting", we can leave it empty;
for final status "Aborted", "Failed", "Succeeded", we may need add additional specific information in the future.
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.
Couple of naming convention spotted but bypassing those for now. Looking good otherwise.
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.
Just heads up; this file is deprecated and superseded by JSON Schema in main
branch. When you rebase to main
, pls see if you can adapt to the latest arrangement. It should be the same meaning in schema-wise.
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.
sure, will deleted the deprecated PayloadDataSucceeded
file.
Update translator service to bclconvert manager
As discussed,
new schema example