Skip to content
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

Remove non-public images from the git-sha-based target determination #525

Merged
merged 4 commits into from
Apr 19, 2023

Conversation

VJalili
Copy link
Member

@VJalili VJalili commented Apr 13, 2023

This PR excludes the non-public docker images (melt specifically`) from the git-sha-based target images determination.

These images can still be built if listed explicitly via the --targets argument.

@VJalili VJalili requested a review from cwhelan April 13, 2023 17:39
@VJalili VJalili marked this pull request as ready for review April 13, 2023 17:39
Copy link
Member

@cwhelan cwhelan left a comment

Choose a reason for hiding this comment

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

I think it's right to exclude these from automatic builds based on dependencies, since you need to provide the non-public melt binary file. To avoid confusion I think it might be good to print a quick warning about this (eg something like "Warning: Skipping build of non-public melt docker even though dependencies have changed; please update these images manually") -- what do you think?

@VJalili
Copy link
Member Author

VJalili commented Apr 13, 2023

Thank you for reviewing this, @cwhelan! I agree printing a warning message would be helpful. I updated accordingly.

Copy link
Member

@cwhelan cwhelan left a comment

Choose a reason for hiding this comment

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

LGTM now.

@VJalili
Copy link
Member Author

VJalili commented Apr 13, 2023

I updated the script to the non-public images (melt specifically) that are also skipped from the dependency tree. Also, I added print statements that indicate the images are omitted.

One way to test this would be the following command, and you can check if the non-pub images are to be built by considering the shown print statement.

$ python scripts/docker/build_docker.py --targets sv-base-virtual-env --disable-git-protect
...
Building the following targets in order:
['sv-base-virtual-env', 'cnmops-virtual-env', 'sv-pipeline-virtual-env', 'sv-base', 'cnmops', 'sv-pipeline']

Note that melt is not included in the above list.

@cwhelan
Copy link
Member

cwhelan commented Apr 19, 2023

Looks good to me.

@VJalili VJalili merged commit c248224 into main Apr 19, 2023
@VJalili VJalili deleted the vj-remove-metl-from-sha-based-target branch April 19, 2023 23:23
@VJalili
Copy link
Member Author

VJalili commented Apr 19, 2023

Thanks, @cwhelan!

gatk-sv-bot pushed a commit to Genometric/gatk-sv that referenced this pull request Jun 7, 2023
…roadinstitute#525)

* Remove melt from sha-based target determination

* Print a warning message for skipped docker images.

* Exclude non-public images from the dependency chain.

* Update removing non-pub images so the skipped images are logged.
gatk-sv-bot pushed a commit to Genometric/gatk-sv that referenced this pull request Jun 27, 2023
…roadinstitute#525)

* Remove melt from sha-based target determination

* Print a warning message for skipped docker images.

* Exclude non-public images from the dependency chain.

* Update removing non-pub images so the skipped images are logged.
gatk-sv-bot pushed a commit to Genometric/gatk-sv that referenced this pull request Jun 27, 2023
…roadinstitute#525)

* Remove melt from sha-based target determination

* Print a warning message for skipped docker images.

* Exclude non-public images from the dependency chain.

* Update removing non-pub images so the skipped images are logged.
MattWellie added a commit to populationgenomics/gatk-sv that referenced this pull request Jul 26, 2023
* Revert to CPG commit ca70123

* Absorb Broad upstream changes:

* Add handling for Flag vcf fields to vcf_to_pandas (broadinstitute#506)
* add outlier samples list & count outputs to PlotSVCountsPerSample (broadinstitute#510)
* eliminate cram to bam conversion when possible (broadinstitute#468)
* Add ref panel inputs for MakeCohortVcf subworkflows (broadinstitute#517)
* Extend STR workflow to collect additional locus-level metrics (broadinstitute#516)
* Change ref allele to N if unsupported in gatk formatting script (broadinstitute#511)
* Add sample renaming for SD files in GatherBatchEvidence (broadinstitute#519)
* Remove vcf header contig sorting in CleanVcf5 (broadinstitute#518)
* Add support for building dockers for multiple registries. (broadinstitute#507)
* Remove non-public images from the git-sha-based target determination (broadinstitute#525)
* Fix ReadMe for build docker (broadinstitute#528)
* Fix for tiny shard of IntegrateGQ in single sample pipeline (broadinstitute#524)
* Fix issue in Genotype batch script with low PE_log_pval causing zero PE_count (broadinstitute#527)
* Manually install MASS R package in sv-pipeline-virtual-env (broadinstitute#534)
* Fix non-deterministic errors in GetSampleIdsFromVcfTar (broadinstitute#531)
* Add ApplyManualVariantFilter json template (broadinstitute#536)
* make the RunMELT task a little more robust (broadinstitute#529)
* Improve STR workflow (broadinstitute#539)
* Fix workspace format (Generate Terra workspace tsv files from transposed tables) (broadinstitute#523)
* Update VaPoR workflows (broadinstitute#532)
* Update README to link to SV callers used. (broadinstitute#541)

Co-authored-by: Mark Walker <markw@broadinstitute.org>
Co-authored-by: epiercehoffman <epierceh@broadinstitute.org>
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.

2 participants