[BUG] keep working directory on failed run / flag to keep pipeline working directory on success #345

bcmcpher · 2024-08-30T21:37:41Z

Closes Pipeline runner removes work directory regardless of job success / fail #297

Changes proposed in this pull request:

Modified .run_cleanup() method to not remove working files in failed processes to facilitate resuming / restarting.
Added additional flag to PipelineRunner() to optionally keep working files even on success.

Checklist (for reviewers)

PR has an interpretable title with a prefix (e.g. [BUG], [DOC], [ENH], [MAINT])\
PR links to GitHub issue with mention Closes #XXXX
Tests pass
Checks pass

For bug fixes:

There is at least one test that would fail under the original bug conditions

… runs - TODO write test

for more information, see https://pre-commit.ci

codecov · 2024-08-30T21:40:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
nipoppy/cli/parser.py	`100.00% <100.00%> (ø)`
nipoppy/cli/run.py	`95.74% <ø> (ø)`
nipoppy/workflows/runner.py	`100.00% <100.00%> (ø)`

michellewang

This looks good overall. The main thing would be to add a test that checks if the directories are kept when there are failures (even if keep_workdir is False). Otherwise minor comments about log message being redundant with other parts of the code.

michellewang · 2024-08-30T22:05:18Z

nipoppy/cli/parser.py

+def add_arg_keepworkdir(parser: _ActionsContainer) -> _ActionsContainer:
+    """Add a --keep_workdir argument to the parser."""
+    parser.add_argument(
+        "--keep_workdir",


Use kebab-style to be consistent with other CLI args

Suggested change

"--keep_workdir",

"--keep-workdir",

Also should update docstring after this change is made

michellewang · 2024-08-30T22:10:32Z

nipoppy/workflows/runner.py

-            if dpath.exists():
-                self.rm(dpath)
+        if self.n_success == self.n_total:
+            self.logger.info("All pipeline segments completed successfully.")


Don't think this is needed since there is already a feedback message printed (from here)

michellewang · 2024-08-30T22:15:13Z

nipoppy/workflows/runner.py

+        if self.n_success == self.n_total:
+            self.logger.info("All pipeline segments completed successfully.")
+            if not self.keep_workdir:
+                self.logger.info("Removing working / intermediary files.")


Log message probably not needed since BaseWorkflow.rm() already prints a log message (see here)

michellewang · 2024-08-30T22:18:54Z

tests/test_workflow_runner.py

@@ -83,21 +83,23 @@ def test_run_setup(config: Config, tmp_path: Path):
    assert runner.dpath_pipeline_work.exists()


-def test_run_cleanup(tmp_path: Path):
+@pytest.mark.parametrize("keep_workdir", [True, False])
+def test_run_cleanup(tmp_path: Path, keep_workdir):


This looks good but we should also include another test for different values of n_success and n_total (this is also what CodeCov is complaining about. See e.g. this test which sets n_success and n_total to different values with different expected results for each pair

michellewang · 2024-08-30T22:27:41Z

nipoppy/cli/parser.py

+        "--keep_workdir",
+        type=str,
+        required=False,
+        help="Keep pipeline working directory upon success (default: False).",


Instead of (default: False), I would put something like `(default: working directory deleted unless a run failed)".

Also ideally mention that this applies to the pyBIDS database too (if it was created)

…test variable for clarity

michellewang

Thanks @bcmcpher!🧑‍🍳

* [MAINT] Only move PRs to pending review. (#389) * [MAINT] Only move PRs to pending review. The PR states is then handled by the project workflows. * cleanup file from removed workflow. * [BUG] keep working directory on failed run / flag to keep pipeline working directory on success (#345) * added method / test for flag to clear working directory * added --keep_workdir argument to parser; fixed bad spacing in test * removed dev comments * added pipeline runner arg to parser / cli invocation * added logic to run_cleanup for keeping intermediary files from failed runs - TODO write test * finalized cleanup fix and unit test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * added unit test for codecov, addressed log feedback * added pre-commit to env * added config saving to test from new api; renamed fail to success in test variable for clarity --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [pre-commit.ci] pre-commit autoupdate (#398) updates: - [github.com/pre-commit/pre-commit-hooks: v4.6.0 → v5.0.0](pre-commit/pre-commit-hooks@v4.6.0...v5.0.0) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [BUG] Do not pool BIDS and PROC pipelines together when searching pipeline configs (#391) * refactor get_pipeline_version to take a list of pipeline configs * refactor get_pipeline_config to take a list of pipeline configs * add tests for BidsConversionWorkflow * fix sphinx xref warning * remove old nitpick-ignores * [ENH] make fmriprep `--fs-no-reconall` value-key and id more intuitive. (#406) --------- Co-authored-by: Brent McPherson <bcmcpher@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Michelle Wang <tomichellewang@gmail.com>

* [ENH] Add `_id` suffix to `bids_participant` and `bids_session` (#382) * rename bids_participant -> bids_participant_id * rename bids_session -> bids_session_id * [ENH] Make `TRACKER_CONFIG_FILE` part of pipeline step config instead of pipeline config (#352) * set default step name * put tracker config file under PipelineStepConfig instead of PipelineConfig (no longer a list) * add pipeline_step column to bagel * use pipeline step config attributes directly instead of getters * improve pipeline step type annotation and refactor validation * update CLI * update tracking-related docs * Revert "improve pipeline step type annotation and refactor validation" This reverts commit aa2e621. * Revert "use pipeline step config attributes directly instead of getters" This reverts commit 6b2821f. * remove pipeline_step_config and check_pipeline_step * move redundant change * fix tests after merge * only allow tracking at participant-session level * address PR review comment * [API] make --dataset-root and positional arg. (#390) * [API] make --dataset-root and positional arg. * [MAINT] Only move PRs to pending review. (#389) * [MAINT] Only move PRs to pending review. The PR states is then handled by the project workflows. * cleanup file from removed workflow. * positional arg should not hae hyphen. Fixed to pass tests. * [ENH] Update pipeline config layout (#383) * move descriptors * move sample invocations * move tracker files * move sample_pipelines inside examples * update layout * update sample global configs * update dataset init and tests * rename proc -> pipelines in layout * fix docs * [ENH] Use TSV instead of CSV for tabular files (#381) * update JSON files * update loading/saving function calls * rename CSV files to TSV * convert to TSV * fix tests * rename remaining csv file * complete CSV -> TSV conversion * update docs * renamed "comma-separated" to "tab-separated" in docs * add check for CSV input and update tests * fix failing test * explicitly set separator as '\t' in BaseTabular instead of relying on default * [ENH] Update layout (#379) * remove releases and demographics directories * rename bids directory * Revert "rename bids directory" This reverts commit 0ff17c9. * move logs directory * rename sourcedata -> to_bidsify * rename raw_imaging -> to_reorg * change fpath_doughnut * move pybids_db file * move pybids_ignore_patterns directory * move downloads under sourcedata/imaging * add new paths for sourcedata directories * add code directory * move containers directory directly under root * add demographics.csv file to layout * move working directories under scratch/work * rename to_reorg -> pre_reorg * rename to_bidsify -> post_reorg * address PR comments * forgot to save before committing * remove missed mentions of "raw"... * rename bagel.tsv -> imaging_bagel.tsv * [ENH] Rename doughnut columns (#404) rename in_raw_imaging -> in_pre_reorg and in_sourcedata -> in_post_reorg * [DOC] Update file trees in user guide page for DICOM reorg (#408) update file trees * [DOC] Update dataset layout figure (#412) * update layout in figure * update figure based on feedback * remove subdirectories/files under pre-/post-reorg * remove trailing slashes, rename idps, etc. * [ENH] Update bagel columns (#354) * remove `pipeline_starttime` column * rename `session` -> `bids_session` * make tracker robust to bad/incompatible existing bagel * rename bids_session -> bids_session_id * fix MRIQC bagel example and add tests for valid/invalid bagels * csv -> tsv * make bids_participant_id and bids_session_id required by the schema * address comment for test --------- Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com> * [REL] Fix merge conflict for REL 0.3.0 (#417) * [MAINT] Only move PRs to pending review. (#389) * [MAINT] Only move PRs to pending review. The PR states is then handled by the project workflows. * cleanup file from removed workflow. * [BUG] keep working directory on failed run / flag to keep pipeline working directory on success (#345) * added method / test for flag to clear working directory * added --keep_workdir argument to parser; fixed bad spacing in test * removed dev comments * added pipeline runner arg to parser / cli invocation * added logic to run_cleanup for keeping intermediary files from failed runs - TODO write test * finalized cleanup fix and unit test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * added unit test for codecov, addressed log feedback * added pre-commit to env * added config saving to test from new api; renamed fail to success in test variable for clarity --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [pre-commit.ci] pre-commit autoupdate (#398) updates: - [github.com/pre-commit/pre-commit-hooks: v4.6.0 → v5.0.0](pre-commit/pre-commit-hooks@v4.6.0...v5.0.0) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * [BUG] Do not pool BIDS and PROC pipelines together when searching pipeline configs (#391) * refactor get_pipeline_version to take a list of pipeline configs * refactor get_pipeline_config to take a list of pipeline configs * add tests for BidsConversionWorkflow * fix sphinx xref warning * remove old nitpick-ignores * [ENH] make fmriprep `--fs-no-reconall` value-key and id more intuitive. (#406) --------- Co-authored-by: Brent McPherson <bcmcpher@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Michelle Wang <tomichellewang@gmail.com> --------- Co-authored-by: Michelle Wang <tomichellewang@gmail.com> Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com> Co-authored-by: Brent McPherson <bcmcpher@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

bcmcpher and others added 7 commits August 29, 2024 15:15

added method / test for flag to clear working directory

5e24867

added --keep_workdir argument to parser; fixed bad spacing in test

00488aa

removed dev comments

7d79542

added pipeline runner arg to parser / cli invocation

f86e635

added logic to run_cleanup for keeping intermediary files from failed…

f010653

… runs - TODO write test

finalized cleanup fix and unit test

67f0bd8

[pre-commit.ci] auto fixes from pre-commit.com hooks

14e7b1a

for more information, see https://pre-commit.ci

github-actions bot added the T.0 - Bug Something isn't working label Aug 30, 2024

michellewang self-requested a review August 30, 2024 22:03

michellewang requested changes Aug 30, 2024

View reviewed changes

bcmcpher self-assigned this Oct 3, 2024

bcmcpher added 2 commits October 3, 2024 13:44

added unit test for codecov, addressed log feedback

aa58094

Merge remote-tracking branch 'upstream/main' into wd-clean

9356bf9

bcmcpher force-pushed the wd-clean branch from c279cf9 to 9356bf9 Compare October 4, 2024 20:29

bcmcpher added 2 commits October 4, 2024 16:35

added pre-commit to env

594e9d3

added config saving to test from new api; renamed fail to success in …

d0f8a2d

…test variable for clarity

michellewang approved these changes Oct 4, 2024

View reviewed changes

bcmcpher merged commit 33961a1 into nipoppy:main Oct 4, 2024
10 checks passed

bcmcpher deleted the wd-clean branch October 4, 2024 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BUG] keep working directory on failed run / flag to keep pipeline working directory on success #345

[BUG] keep working directory on failed run / flag to keep pipeline working directory on success #345

Uh oh!

bcmcpher commented Aug 30, 2024

Uh oh!

codecov bot commented Aug 30, 2024 •

edited

Loading

Uh oh!

michellewang left a comment

Uh oh!

michellewang Aug 30, 2024

Uh oh!

michellewang Aug 30, 2024

Uh oh!

michellewang Aug 30, 2024

Uh oh!

michellewang Aug 30, 2024

Uh oh!

michellewang Aug 30, 2024

Uh oh!

michellewang left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[BUG] keep working directory on failed run / flag to keep pipeline working directory on success #345

[BUG] keep working directory on failed run / flag to keep pipeline working directory on success #345

Uh oh!

Conversation

bcmcpher commented Aug 30, 2024

Checklist (for reviewers)

Uh oh!

codecov bot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michellewang left a comment

Choose a reason for hiding this comment

Uh oh!

michellewang Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

michellewang Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

michellewang Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

michellewang Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

michellewang Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

michellewang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 30, 2024 •

edited

Loading

michellewang left a comment •

edited

Loading