Conversation
|
@copilot resolve the merge conflicts in this pull request |
![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: f91b01ec4a
ℹ️ 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".
|
|
||
| ``` | ||
| MEGAHITHR=$(sbatch --parsable --dependency=afterok:$HOSTJOB $PAWSEY_SRC/megahit_hostremoved.slurm) | ||
| MEGAHITHR=$(sbatch --parsable --dependency=afterok:$HOSTJOB $PAWSEY_SRC/megahit_allreads.slurm) |
There was a problem hiding this comment.
When users follow docs/quickstart.md with the documented Pawsey profile, step 9 still submits megahit_hostremoved.slurm. This commit removes that path in favor of megahit_allreads.slurm, so the quick-start assembly submission now fails before a job is created; update that command as part of the rename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Updates the Pawsey short-read assembly workflow documentation and Slurm submission script naming to standardize on megahit_allreads.slurm, and starts documenting an alternative “individual assemblies + merge” path.
Changes:
- Switched job-submission examples in
pawsey_shortread/README.mdfrommegahit_hostremoved.slurmtomegahit_allreads.slurm. - Added a new README subsection describing individual per-sample assemblies followed by a contigger merge step (currently incomplete).
- Adjusted Slurm stdout/stderr output paths in
pawsey_shortread/megahit_allreads.slurm.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pawsey_shortread/README.md | Updates workflow instructions to use megahit_allreads.slurm and adds an (incomplete) individual-assembly + contigger merge subsection. |
| pawsey_shortread/megahit_allreads.slurm | Changes Slurm log output/error paths for the megahit_allreads job. |
Suppressed comments (2)
pawsey_shortread/megahit_allreads.slurm:9
- This script writes Slurm logs directly under
slurm_output/, but other MEGAHIT scripts in the repo (includingpawsey_shortread/megahit.slurm) useslurm_output/megahit_slurm/. Keeping the same subdirectory avoids mixing logs from different steps and matches existingmkdir -p slurm_output/megahit_slurminstructions elsewhere.
pawsey_shortread/README.md:278 - This paragraph says the
--continueflag should be active, but inmegahit_allreads.slurmthe--continueinvocation is currently commented out. Reword this to instruct users to uncomment--continuewhen resuming an interrupted run (and fix the typo “interuppted”).
Assemble using the `megahit_allreads.slurm` script above. This will take a while to run, so do it early! Also note that `megahit` can continue if it is interuppted. Make sure the `--continue` flag is active
in the `megahit_allreads.slurm` script.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Then, merge the assemblies with contigger: | ||
|
|
||
| ``` | ||
| TBD! |
|
|
||
| ``` | ||
| MEGAHITHR=$(sbatch --parsable --dependency=afterok:$HOSTJOB $PAWSEY_SRC/megahit_hostremoved.slurm) | ||
| MEGAHITHR=$(sbatch --parsable --dependency=afterok:$HOSTJOB $PAWSEY_SRC/megahit_allreads.slurm) |
Co-authored-by: linsalrob <836231+linsalrob@users.noreply.github.com>
Resolved by merging |
Co-authored-by: linsalrob <836231+linsalrob@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pawsey_shortread/README.md:153
- This section introduces an optional contigger merge workflow but leaves a
TBD!placeholder inside a code block. As-is, users can’t run the documented workflow; please either add the actual contigger command(s) and expected inputs/outputs, or remove this subsection until it’s ready.
Then, merge the assemblies with contigger:
TBD!
pawsey_shortread/README.md:278
- Typo in the documentation:
interupptedshould beinterrupted(and this sentence currently has a trailing space afteractive).
Assemble using the `megahit_allreads.slurm` script above. This will take a while to run, so do it early! Also note that `megahit` can continue if it is interuppted. Make sure the `--continue` flag is active
in the `megahit_allreads.slurm` script.
This pull request updates the assembly workflow documentation and scripts to use the
megahit_allreads.slurmscript instead ofmegahit_hostremoved.slurm, and adds instructions for performing individual assemblies and merging them. The changes also include a file rename and minor output path adjustments.Workflow and documentation updates:
README.mdto use the newmegahit_allreads.slurmscript instead ofmegahit_hostremoved.slurm. [1] [2] [3]README.md.Script and file changes:
megahit_hostremoved.slurmtomegahit_allreads.slurmand updated output/error log paths toslurm_output/%x-%j.outandslurm_output/%x-%j.err.