More updates - #33
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d66ebfd83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #SBATCH --mem=32G | ||
| #SBATCH -o slurm/human_download-%j.out | ||
| #SBATCH -e slurm/human_download-%j.err | ||
| #SBATCH -o slurm/%x-%j.out |
There was a problem hiding this comment.
When following either Pawsey README in a fresh analysis directory, the setup creates only slurm_output/..., while all eight database-download scripts now direct stdout and stderr into slurm/. Slurm connects --output to the specified filename before the batch script runs (official sbatch documentation), so without a pre-existing slurm directory these jobs fail during I/O setup and none of the databases are downloaded; add this directory to the documented setup or target the existing slurm_output tree.
Useful? React with 👍 / 👎.
| exit 0 | ||
| fi | ||
|
|
||
| curl -LO https://ftp.ncbi.nlm.nih.gov/genomes/all/GCF/000/001/405/GCF_000001405.40_GRCh38.p14/GRCh38_major_release_seqs_for_alignment_pipelines/GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz |
There was a problem hiding this comment.
If this transfer is interrupted or receives an HTTP error, the script has neither set -e nor curl's --fail behavior (curl documentation), so it proceeds to the final echo and reports a successful Slurm job. Because -O writes directly to the final filename and the earlier guard checks only existence, a partial or error-response file is then accepted as a complete human database on every retry; propagate curl failures and download atomically before exposing the final filename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates Pawsey workflow documentation and SLURM scripts to improve reproducibility and standardize operational behavior, including new database download jobs and updates to VAMB job environments.
Changes:
- Added new
pawsey_minionSLURM scripts to download required databases (human reference, NCBI taxonomy, UniRef50/UniRef100). - Updated VAMB-related SLURM jobs to use the
atavide_lite_vambconda environment and refreshed the PyTorch module for GPU runs. - Standardized/adjusted job logging and timings (e.g., download script log locations; reduced
mmseqs_easy_taxonomy100walltime).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pawsey_shortread/README.md | Adds a reminder to build executables before running the workflow. |
| pawsey_shortread/mmseqs_easy_taxonomy100.slurm | Reduces requested walltime for the UniRef100 easy-taxonomy job. |
| pawsey_shortread/download_uniref50.slurm | Updates SLURM stdout/stderr paths for UniRef50 download job. |
| pawsey_shortread/download_uniref100.slurm | Updates SLURM stdout/stderr paths for UniRef100 download job. |
| pawsey_shortread/download_taxon_db.slurm | Updates SLURM stdout/stderr paths for NCBI taxonomy download job. |
| pawsey_shortread/download_human.slurm | Updates SLURM stdout/stderr paths for human reference download job. |
| pawsey_minion/vamb.slurm | Switches to atavide_lite_vamb, updates PyTorch module, and avoids regenerating existing BAMs. |
| pawsey_minion/vamb_concat.slurm | Switches to atavide_lite_vamb for the concatenate step. |
| pawsey_minion/README.md | Adds build reminder; cleans up sbatch examples by removing unnecessary --export usage. |
| pawsey_minion/download_uniref50.slurm | New UniRef50 download SLURM job with file-presence checks. |
| pawsey_minion/download_uniref100.slurm | New UniRef100 download SLURM job with file-presence checks. |
| pawsey_minion/download_taxon_db.slurm | New NCBI taxonomy download SLURM job with file-presence checks. |
| pawsey_minion/download_human.slurm | New human reference download SLURM job with file-presence checks. |
Suppressed comments (4)
pawsey_minion/download_uniref50.slurm:63
- Temporary directory created by
mktemp -dis never cleaned up. Repeated runs will leave many leftover directories under$TMP, which can consume space/quota on shared filesystems.
TPD=$(mktemp -d -p $TMP)
echo -e "Downloading to $DESTINATION/$DB and using temp location $TPD.\nNote we download the tarball and then uncompress, so there is usually nothing in the output for a while!" >&2
pawsey_minion/download_uniref100.slurm:62
- Temporary directory created by
mktemp -dis never cleaned up. Repeated runs will leave many leftover directories under$TMP, which can consume space/quota on shared filesystems.
TPD=$(mktemp -d -p $TMP)
echo -e "Downloading to $DESTINATION/$DB and using temp location $TPD.\nNote we download the tarball and then uncompress, so there is usually nothing in the output for a while!" >&2
pawsey_minion/download_human.slurm:19
- When the human reference file already exists, the script
touches it and exits. This can mask partially downloaded/corrupted files (existence alone doesn’t imply validity) and it needlessly changes the mtime. A safer check is to skip only when the file is non-empty, and avoid touching it.
if [[ -e GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz ]]; then
echo "GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz already exists. Nothing to do";
touch GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz;
exit 0
fi
pawsey_minion/download_taxon_db.slurm:33
curl -LOwill happily write an HTML error page (e.g., transient 5xx) intotaxdump.tar.gzand the script will then attempt to extract it. Using-fL(and optionally retries) makes failures explicit and avoids silently producing a bad local cache.
echo "One or more files are missing. Downloading taxdump.tar.gz..."
curl -LO https://ftp.ncbi.nih.gov/pub/taxonomy/taxdump.tar.gz
tar zxf taxdump.tar.gz && rm -f taxdump.tar.gz
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for READS in $(sort -R reads.txt); do | ||
| BAM=${READS/$FILEEND/.bam} | ||
| echo "READS: $READS BAM: $BAM" >&2; | ||
| if [[ -e $OUTDIR/mapped_reads/$BAM ]]; then | ||
| if [[ ! -e $OUTDIR/mapped_reads/$BAM ]]; then | ||
| echo "Bam file: '$OUTDIR/mapped_reads/$BAM' not found. Data generated" >&2; |
There was a problem hiding this comment.
Fixed — added mkdir -p $OUTDIR/mapped_reads inside the if [[ ! -e ... ]] block before the minimap2/samtools pipeline runs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (9)
pawsey_shortread/README.md:20
- Typo in path:
atavide_lit/binshould beatavide_lite/bin(matches the actual directory used in the code block below).
Make sure you build atavide_lit/bin:
pawsey_minion/README.md:26
- Typo in path:
atavide_lit/binshould beatavide_lite/bin(matches the actual directory used in the code block below).
Make sure you build atavide_lit/bin:
pawsey_shortread/download_uniref100.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_shortread/download_taxon_db.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_shortread/download_uniref50.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_shortread/download_human.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_minion/download_uniref50.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_minion/download_uniref100.slurm:8
#SBATCH -o/-e slurm/%x-%j.*requires theslurm/directory to exist before the job starts; since SBATCH directives are applied before the script runs, this can cause the job to fail to open its output/error files whenslurm/hasn't been created in the submit directory.
#SBATCH -o slurm/%x-%j.out
#SBATCH -e slurm/%x-%j.err
pawsey_minion/vamb.slurm:44
- This block can attempt to write
$OUTDIR/mapped_reads/$BAMwithout ensuring$OUTDIR/mapped_reads/exists. With the updated condition, the minimap2/samtools pipeline will now run when the BAM is missing, and will fail if themapped_readsdirectory hasn't been created (e.g., when runningvamb.slurmstandalone).
if [[ ! -e $OUTDIR/mapped_reads/$BAM ]]; then
echo "Bam file: '$OUTDIR/mapped_reads/$BAM' not found. Data generated" >&2;
minimap2 -t 16 -N 5 -ax map-ont $OUTDIR/contigs.mmi --split-prefix mmsplit$$ fastq/$READS | samtools view -F 3584 -b --threads 16 | samtools sort -@ 16 -o $OUTDIR/mapped_reads/$BAM -
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (8)
pawsey_shortread/download_uniref50.slurm:5
- This job requests
--cpus-per-task=1but later runsmmseqs databases --threads 8. To avoid oversubscription, align the CPU request with the configured thread count (or lower--threads).
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
pawsey_minion/download_uniref100.slurm:5
mmseqs databasesis invoked with--threads 8, but the job requests only--cpus-per-task=1. This can oversubscribe the allocated CPU resources and may violate scheduler policies. Request 8 CPUs (or reduce threads).
#SBATCH --time=1-0
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
pawsey_minion/download_uniref50.slurm:5
mmseqs databasesis invoked with--threads 8, but the job requests only--cpus-per-task=1. This can oversubscribe the allocated CPU resources and may violate scheduler policies. Request 8 CPUs (or reduce threads).
#SBATCH --time=1-0
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
pawsey_minion/vamb.slurm:18
- This script uses 16-way CPU parallelism (
minimap2 -t 16,samtools ... --threads 16,vamb -p 16) but does not request CPUs from Slurm (no--ntasks/--cpus-per-task). That can lead to severe oversubscription or unpredictable performance. Request CPUs explicitly (and consider wiring the thread counts to$SLURM_CPUS_PER_TASK).
# test we have atavide_lite conda installed, and then activate it
eval "$(conda shell.bash hook)"
"$HOME/GitHubs/atavide_lite/pawsey_lib/check_atavide_lite_env.sh"
ATAVIDE_CONDA="/scratch/$PAWSEY_PROJECT/$USER/software/miniconda3/atavide_lite_vamb"
conda activate $ATAVIDE_CONDA
pawsey_minion/download_taxon_db.slurm:9
- This download script doesn't enable strict error handling. If
curlortarfails, the job can still exit successfully and leave partial/corrupt data behind. Most other pipeline scripts useset -euo pipefail+ anERRtrap; add the same here.
#SBATCH --mem=32G
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
TAXDIR="/scratch/$PAWSEY_PROJECT/$USER/Databases/NCBI/taxonomy/current"
pawsey_shortread/download_taxon_db.slurm:9
- This download script doesn't enable strict error handling. If
curlortarfails, the job can still exit successfully and leave partial/corrupt data behind. Most other pipeline scripts useset -euo pipefail+ anERRtrap; add the same here.
#SBATCH --mem=32G
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
TAXDIR="/scratch/$PAWSEY_PROJECT/$USER/Databases/NCBI/taxonomy/current"
pawsey_minion/download_human.slurm:22
- This script lacks strict error handling; if the download fails, it can still print a success message and exit 0. Add
set -euo pipefail+ anERRtrap, and usecurl -fso HTTP errors fail the job.
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
DEST=/scratch/$PAWSEY_PROJECT/$USER/Databases/human
pawsey_shortread/download_human.slurm:10
- This script lacks strict error handling; if the download fails, it can still print a success message and exit 0. Add
set -euo pipefail+ anERRtrap, and usecurl -fso HTTP errors fail the job.
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
DEST=/scratch/$PAWSEY_PROJECT/$USER/Databases/human
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (12)
pawsey_minion/download_taxon_db.slurm:33
curl -LOwill happily write an HTML error page (e.g. 404) and return success unless-fis used, which can lead to extracting a non-tarball and leaving missing taxonomy files.
echo "One or more files are missing. Downloading taxdump.tar.gz..."
curl -LO https://ftp.ncbi.nih.gov/pub/taxonomy/taxdump.tar.gz
tar zxf taxdump.tar.gz && rm -f taxdump.tar.gz
pawsey_minion/download_human.slurm:22
- When the genome file already exists,
touchupdates its mtime even though nothing was downloaded. Also,curl -LOdoesn't fail the job on HTTP errors (without-f), so the script can print success even if the download failed.
if [[ -e GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz ]]; then
echo "GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz already exists. Nothing to do";
touch GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz;
exit 0
fi
curl -LO https://ftp.ncbi.nlm.nih.gov/genomes/all/GCF/000/001/405/GCF_000001405.40_GRCh38.p14/GRCh38_major_release_seqs_for_alignment_pipelines/GCA_000001405.15_GRCh38_no_alt_plus_hs38d1_analysis_set.fna.gz
echo "Downloaded human db to $DEST"
pawsey_shortread/download_taxon_db.slurm:9
- This download script doesn't use fail-fast shell options. If
curl/tarfails, the job can still print "Done" and exit 0, leaving an incomplete taxonomy DB behind.
pawsey_shortread/download_human.slurm:9 - This script lacks
set -euo pipefail/traphandling, so a failedcurlcan still result in the job finishing and printing a success message.
pawsey_shortread/download_uniref50.slurm:6 - This job requests 1 CPU (
--cpus-per-task=1) but runsmmseqs databases --threads 8later in the script. That can oversubscribe the node and/or violate scheduler policy.
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
pawsey_minion/download_uniref50.slurm:6
- This job requests 1 CPU (
--cpus-per-task=1) but runsmmseqs databases --threads 8. That can oversubscribe the node and/or violate scheduler policy.
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
pawsey_minion/download_uniref100.slurm:6
- This job requests 1 CPU (
--cpus-per-task=1) but runsmmseqs databases --threads 8. That can oversubscribe the node and/or violate scheduler policy.
#SBATCH --ntasks=1
#SBATCH --cpus-per-task=1
#SBATCH --mem=128G
pawsey_shortread/download_taxon_db.slurm:8
- The PR description says SLURM logs were standardized to
slurm/%x-%j.outandslurm/%x-%j.err, but this script (and others in the PR) now writes to%x-%j.out/%x-%j.errin the submission directory. Either the PR description should be updated, or the scripts/docs should be adjusted to consistently use a dedicated log directory (ensuring it exists beforesbatch).
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
pawsey_shortread/download_human.slurm:8
- The script uses
curl -LO(which doesn't fail the job on HTTP errors without-f) andtouches the genome file when it already exists, updating its mtime even though nothing was downloaded.
This issue also appears on line 9 of the same file.
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
pawsey_shortread/download_taxon_db.slurm:8
- This script uses
curl -LOfor the NCBI tarball. Without-f,curlcan return success even for HTTP errors (saving an HTML error page), which can lead totarextracting garbage and leaving missing taxonomy files.
This issue also appears on line 9 of the same file.
#SBATCH -o %x-%j.out
#SBATCH -e %x-%j.err
pawsey_minion/download_taxon_db.slurm:9
- This download script doesn't use fail-fast shell options. If
curl/tarfails, the job can still print "Done" and exit 0, leaving an incomplete taxonomy DB behind.
This issue also appears on line 31 of the same file.
pawsey_minion/download_human.slurm:9
- This script lacks
set -euo pipefail/traphandling, so a failedcurlcan still result in the job finishing and printing a success message.
This issue also appears on line 15 of the same file.
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e16ec4537a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| curl -LO https://ftp.ncbi.nih.gov/pub/taxonomy/taxdump.tar.gz | ||
| tar zxf taxdump.tar.gz && rm -f taxdump.tar.gz |
There was a problem hiding this comment.
If the taxonomy transfer is interrupted or the archive is corrupt, curl or tar can return nonzero, but this script continues to the final echo "Done" and exits successfully. Slurm will therefore report a completed download even though required taxonomy files may be missing or partially extracted; make the download/extraction failure terminate the script and only expose validated files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Added fail-fast shell behavior and safely quoted taxonomy database paths in both Pawsey workflows. pawsey_minion/download_taxon_db.slurmL10-L15 pawsey_shortread/download_taxon_db.slurmL10-L15
- Updated existing-file checks to reject missing or empty taxonomy files. pawsey_minion/download_taxon_db.slurmL23-L30 pawsey_shortread/download_taxon_db.slurmL23-L30
- Made taxonomy downloads fail on HTTP errors, retry transient failures, and extract into a temporary staging directory with automatic cleanup. pawsey_minion/download_taxon_db.slurmL32-L41 pawsey_shortread/download_taxon_db.slurmL32-L41
- Added validation of every required extracted file before publishing the downloaded database into its final location. pawsey_minion/download_taxon_db.slurmL43-L53 pawsey_shortread/download_taxon_db.slurmL43-L53
- Committed the changes as
71381f8(Harden taxonomy database downloads) and created the follow-up pull request titled Harden taxonomy database downloads.
Testing
- ✅
git diff --check - ✅
bash -n pawsey_minion/download_taxon_db.slurm pawsey_shortread/download_taxon_db.slurm ⚠️ shellcheck pawsey_minion/download_taxon_db.slurm pawsey_shortread/download_taxon_db.slurm(ShellCheck is not installed in the environment.)- ✅
git status --porcelain(working tree clean after commit)
|
@codex fix comments |
Summary
Testing
|
This pull request introduces several improvements and fixes to the pipeline documentation and SLURM job scripts for both the
pawsey_minionandpawsey_shortreadworkflows. The main focus is on enhancing reproducibility, standardizing job outputs, updating environment usage, and adding missing database download scripts.Key changes include:
New Database Download Scripts
download_human.slurm), NCBI taxonomy (download_taxon_db.slurm), UniRef50 (download_uniref50.slurm), and UniRef100 (download_uniref100.slurm). These scripts check for existing files and only download missing data, improving reproducibility and automation. [1] [2] [3] [4]Environment and Module Updates
atavide_lite_vambfor bothvamb.slurmandvamb_concat.slurm, ensuring the correct dependencies are used. [1] [2]vamb.slurmto a newer version (2.7.1-rocm6.3.3) for improved compatibility and performance.SLURM Job Script Standardization
slurm/%x-%j.outandslurm/%x-%j.erracross all download scripts for easier log management. [1] [2] [3] [4]Pipeline Documentation and Logic Fixes
pawsey_minion/README.mdandpawsey_shortread/README.mdto build theatavide_lite/binexecutables before running the pipeline. [1] [2]--export=ATAVIDE_CONDA=$ATAVIDE_CONDAwhere not required, and corrected job dependencies. [1] [2] [3]vamb.slurmso that BAM files are only generated if they do not already exist, preventing unnecessary recomputation.Resource Allocation Adjustment
mmseqs_easy_taxonomy100.slurmfrom 1 day to 6 hours, likely reflecting more accurate job duration requirements.