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.

Sign up for GitHub

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

Jump to bottom

Dev #282

Merged
merged 6 commits into from
Jul 31, 2024
Merged

Dev #282

merged 6 commits into from
Jul 31, 2024

Conversation

bjlang
Copy link

@bjlang bjlang commented Jul 26, 2024 •
edited
Loading

Copying the matrix as feature_matrix seems to have made the inputs appear to differ between identical runs, thus preventing caching.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/differentialabundance branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@bjlang bjlang requested a review from WackerO July 26, 2024 14:28
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 26, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 0e6b4fc

+| ✅ 294 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-07-30 13:12:55

@bjlang bjlang closed this Jul 26, 2024
@bjlang bjlang reopened this Jul 26, 2024
@WackerO
Copy link
Collaborator

WackerO commented Jul 29, 2024

Hello there, I think the caching fails not because of a different md5sum but because the copied matrix has another file name. Not sure exactly how the nextflow resume functionality works, but I assume it takes file names into consideration.
In any case, we cannot just remove this line from the code. The file copying is actually necessary for the shinyngs validator to run.

If we want the resume to work in these cases, we need to find another solution. Closing this for now; feel free to re-open if you have another idea :)

@WackerO WackerO closed this Jul 29, 2024
@WackerO WackerO reopened this Jul 30, 2024
@bjlang
Copy link
Author

bjlang commented Jul 30, 2024 •
edited
Loading

I believe the problem is rather the different timestamps of creation.
That's why I suggest to skip copying, iff this was copied by the previous run, i.e. the file exists AND the content of the file would be identical to the copied one.
A caveat I se is that the suggestion actually would still only allow resuming in case someone runs the pipeline multiple times consecutively on the same input matrix, e.g. nextflow run main.nf -resume -profile test_nogtf,docker --outdir result_test, but the cache will still become invalidated everytime one uses a different input matrix. I guess this could be tackled by actually using different file names, because currently the filename is actually always the same (matrix_as_anno.tsv).

@WackerO
Copy link
Collaborator

WackerO commented Jul 30, 2024

@bjlang As already discussed on slack, I didn't understand the code change correctly initially; thanks for the contribution! I'm approving, please first commit the suggestion that I made to the changelog before merging 😄

WackerO
WackerO approved these changes Jul 30, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@bjlang @WackerO
Update CHANGELOG.md
0e6b4fc 
Co-authored-by: WackerO <[email protected]>
@WackerO WackerO merged commit c44e6bc into nf-core:dev Jul 31, 2024
12 checks passed
@WackerO
Copy link
Collaborator

WackerO commented Jul 31, 2024

Thanks for the work, @bjlang!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WackerO WackerO WackerO approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bjlang @WackerO