Strip inline comments from #$ directives so later qsub options are not dropped - #57
Open
eastagiletracker wants to merge 1 commit into
Conversation
The options from each #$ directive are joined into a single qsub_options field, which pkgtest.nf passes to SGE as one clusterOptions line. A '#' left on one directive therefore comments out every option that follows it, silently dropping those resource requests. The comment is now removed from each directive line before the directives are joined. Adds unit tests for extract_qsub_opts() covering the comment handling, the uncommented case, and the ignored -j/-P/-N flags.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes stripping inline comments from
#$directives infind_qsub.pyso that a commented directive no longer silences the resource requests that follow it (issue #49). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/316. You can sign in with your GitHub ID to claim ownership of the project.What changed and why
SccModule.extract_qsub_opts()reads each#$line from atest.qsuband joins them all into the singleqsub_optionsCSV column, whichpkgtest.nfthen hands to SGE as oneclusterOptionsline. As issue #49 records, SGE ignores everything after a#on that line — so once the directives are joined, a comment on an early directive comments out every option after it. In theshapeit/5.1.1case from that issue,-l avx512was dropped and the test ran on a node without those instructions.The fix removes each directive's comment while the directives are still on separate lines, which is the same per-line reading SGE applies to the file itself. A directive line that is entirely a comment now contributes nothing instead of an empty entry. Directives without comments produce byte-identical output, and the existing
-j/-P/-Nfiltering is unchanged, so nothing already working moves.This is worth doing in the tooling as well as in the guidance already noted on the issue, because the failure is silent and the existing
qsub -w pvalidation cannot catch it: SGE readstest.qsubline by line, where the comment is harmless, so the file validates as good and the corruption appears only after the join. There is a second effect the same#causes —gpu_filter_tests()matches-l gpusanywhere in that joined string, so comment text mentioning GPUs can misroute a test under--gpus-only/--no-gpus.Reproduction on
mainatc5a5f00, with atest.qsubholding the two directives quoted in the issue:Feeding that column through the real
to_csv_rows()/save_csv()writer and reading it back the waypkgtest.nfdoes gives theclusterOptionsline-P rcstest -N nf_shapeit_5.1.1 -pe omp 4 # Use 4 CPUs -l avx512 # ..., of which SGE parses only-P rcstest -N nf_shapeit_5.1.1 -pe omp 4. With this change the same end-to-end run yields-P rcstest -N nf_shapeit_5.1.1 -pe omp 4 -l avx512.Verification.
tests/test_extract_qsub_opts.pycovers the commented case, the uncommented case, the ignored-flag filtering, and a directive commented out in place. Two of the four fail on the current tree (assert '-pe omp 4 # Use 4 CPUs -l avx512 # ...' == '-pe omp 4 -l avx512') and all four pass with the change; the other two are controls that pass in both states, which is what shows existing behavior is untouched. There is no test harness or CI onmaintoday, so the before/after comparison is that file pluspython3 -m py_compile scripts/find_qsub.py, both clean before and after — no new failures. The tests are plain pytest functions in atests/directory and add no dependency beyond whatfind_qsub.pyalready imports, so they sit alongside the suite proposed in #53 rather than competing with it.#$directives are left intest.qsubfiles untouched; only the extracted option string changes.How this was managed
We imported your issues and pull requests onto a live agile board — 56 stories — and worked this change on the story for issue #49, https://eastagiletracker.com/projects/316/stories/204444, with the rest of the history on the board at https://eastagiletracker.com/projects/316.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com