Skip to content

Conversation

@maggiecbms
Copy link
Collaborator

This PR includes changes to add Omics support.

@golharam
Copy link

I'm okay with merging this in. There are a couple of TODOs in the code but it seems to be working okay.

@maggiecbms maggiecbms requested a review from golharam April 15, 2025 14:57
@golharam
Copy link

golharam commented May 6, 2025

I think this PR is almost ready to go. The one thing I don't like is that launchers can be run with the '--project_name' field, but when submitting tasks, a 'project_id' aka run group id is expected so there is still a little discrepancy that needs to be addressed.

@golharam
Copy link

golharam commented May 7, 2025

The last thing I notice is there are references to 'Output', and 'output'. Additionally files are uploaded to /outputs/jobid/inputs/....
It's a little confusing. For instance, upload_file, the target_path is "Outputs/[run_group_id]/launcher_inputs/[filename]"

Maybe we shouldn't use "Outputs" as a folder name as 'output' is used by omics for the workflow outputs.

@maggiecbms
Copy link
Collaborator Author

I think this PR is almost ready to go. The one thing I don't like is that launchers can be run with the '--project_name' field, but when submitting tasks, a 'project_id' aka run group id is expected so there is still a little discrepancy that needs to be addressed.

Unlike SBG and Arvaods, Omics allows two projects (run groups) to have identical names, so the get_project_by_name() function may find multiple run group IDs. I think using project ID would be better than using project name when using Omics.

I can work on get_project_by_name() to return the first element, so that launcher can run. But it does have a potential issue when another run group exists.

@maggiecbms
Copy link
Collaborator Author

The last thing I notice is there are references to 'Output', and 'output'. Additionally files are uploaded to /outputs/jobid/inputs/.... It's a little confusing. For instance, upload_file, the target_path is "Outputs/[run_group_id]/launcher_inputs/[filename]"

Maybe we shouldn't use "Outputs" as a folder name as 'output' is used by omics for the workflow outputs.

Currently output files of a run goes into s3://{Bucket}/Outputs/{RunGroupID}/{WorkflowID}/{TaskName}/{RunID}/ , and the upload_file() uploads files into s3://{Bucket}/Outputs/{RunGroupID}/launcher_inputs/ .

I can change the output folder to s3://{Bucket}/Project/{RunGroupID}/{WorkflowID}/{TaskName}/{RunID}/ and upload file to s3://{Bucket}/Project/{RunGroupID}/launcher_inputs/ .

'''
Create project
'''
self.api.create_run_group(name=project_name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return a dict where { 'runGroupId': XYZ }
?

Copy link

@golharam golharam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good:

  1. Change references to ProjectId to runGroupId to remove any confusion of terms
  2. Ask Claude 3.7 to implement unit tests

After that, we are good.

@maggiecbms
Copy link
Collaborator Author

This looks good:

  1. Change references to ProjectId to runGroupId to remove any confusion of terms
  2. Ask Claude 3.7 to implement unit tests

After that, we are good.

I have replaced the ProjectId to RunGroupId in omics_platform.py.
Unit test is also added with help of Claude.

SUPPORTED_PLATFORMS = {
'Arvados': ArvadosPlatform,
# 'Omics': OmicsPlatform,
'Omics': OmicsPlatform,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the name from 'Omics' to 'AWSHealthOmics' to better reflect the real name? I think 'Omics' was just easier to say and type

self.s3_client = boto3.client('s3')

# WES API connection parameters
self.wes_url = kwargs.get('wes_url', os.getenv('WES_URL'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this WES_API_ENDPOINT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants