-
Notifications
You must be signed in to change notification settings - Fork 2
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
Resolve Issue #9 (Reorganizing dockerfiles and docker images CI/CD) #26
Conversation
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.
Overall comments:
- How about we rename the steps of the workflow as download, convertInput, inference, pyradiomics? If later we decide to add other segmentation tools, we can rename inference step to be tool-specific, but we can do it later.
- Can you please update other workflows to have trigger on PR?
- I think we may also want to have a trigger on commit to the main branch, and push docker images to DockerHub/GHR only on main commits. I am not sure we should push images from the PRs until those are finalized.
id: https://cgc-api.sbgenomics.com/v2/apps/vamsikrishna14/idc/totalsegmentatorend-to-end/1/raw/ | ||
sbg:id: vamsikrishna14/idc/totalsegmentatorend-to-end/1 | ||
sbg:revision: 1 | ||
id: https://cgc-api.sbgenomics.com/v2/apps/vamsikrishna14/idc/totalsegmentatorend-to-end/2/raw/ |
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.
Can you explain the meaning of this line?
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.
I generate this file off of the workflows from SB-CGC unlike WDL files, where we write the code ourselves.
The link is referencing the app in my project.
sbg:image_url: | ||
sbg:appVersion: | ||
- v1.2 | ||
sbg:id: vamsikrishna14/idc/downloaddicomandconvert/2 | ||
sbg:revision: 2 | ||
sbg:id: vamsikrishna14/idc/downloaddicomandconvert/3 |
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.
Another reference to your personal ID - is it needed?
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 an issue was opened just to address references, I created another branch for it, fixed most references already there, and plan to submit a pull request.
for the personal ID, I did not evaluate what part of the cwl file is mandatory and what is not. I will evaluate, clean up and submit the pull request fix referencing #4.
@vkt1414 sounds good - we should describe those points in the documentation under the respective folder of the repo. Will you update the other workflows or should I do it? |
I can add/update the readme.md file with in the cwl files folder, to explain how the CWL file is generated, what we chose to keep and what we stripped. |
We can do this separately, no need to hold this PR. Can you make the other changes I asked - updating workflows as the one I modified, and also pushing the images only when the commit is made to the main branch? Let me know if you need help. |
Sure. I just made changes to github actions files, the same way you edited one yesterday, to rebuild images on main branch only with a pull request. I also renamed the radiomics to dicom_seg_pyradiomics_sr to describe more closely what we are using the docker image for.. I believe these are all the changes related to dockerfiles. |
|
||
push-dockerhub: | ||
needs: build | ||
if: github.event_name == 'push' && github.ref == 'refs/heads/main' |
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.
Nice!
task2 =imagingdatacommons/inference_totalseg
task3 =imagingdatacommons/radiomics
tasks1and2 =imagingdatacommons/download_convert_inference_totalseg
end_to_end =imagingdatacommons/download_convert_inference_totalseg_radiomics
whenever the corresponding dockerfile or the weight_download script or the github actions script itself is modified.
The last four commits spilled are related to other issues. Will isolate branches going forward.