build: replace system c3d dependency with picsl-c3d#93
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces external Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR makes the build and runtime environment more reproducible by removing reliance on a system-installed c3d binary and switching the STAPLE merge implementation to use the Python package picsl-c3d instead.
Changes:
- Add
picsl-c3d==1.4.6.0as a pinned runtime dependency. - Replace
subprocessexecution ofc3dwithpicsl_c3d.Convert3D().execute()in the STAPLE segmentation merger. - Remove the Makefile’s non-reproducible
wget/sudoinstallation path forc3dand simplifyinstall/install-dev.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pyproject.toml |
Pins and adds picsl-c3d as a project dependency to eliminate system c3d requirements. |
oxytcmri/interface/mri/staple_segmenter.py |
Switches STAPLE merging execution from subprocess to picsl-c3d’s Python API. |
makefile |
Removes the system-level c3d install target and updates install workflows to use uv sync. |
| logger.info(f"Running c3d command: {cmd}") | ||
| picsl_c3d.Convert3D().execute(cmd, out=stdout, err=stderr) | ||
| logger.info(f"c3d command completed successfully, output saved to {output_path}") | ||
| except subprocess.CalledProcessError as e: | ||
| raise RuntimeError(f"c3d command failed: {e.stderr}") | ||
| except RuntimeError as e: | ||
| error_details = stderr.getvalue().strip() or stdout.getvalue().strip() or str(e) | ||
| raise RuntimeError(f"c3d command failed: {error_details}") from e |
There was a problem hiding this comment.
The log and raised error text still refer to a “c3d command”, but this code now executes via picsl_c3d.Convert3D(). Updating these messages to mention picsl-c3d/Convert3D would avoid confusion when debugging failures.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@tomboulier I've opened a new pull request, #94, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: update log and error messages to reference picsl-c3d Convert3D Co-authored-by: tomboulier <14161960+tomboulier@users.noreply.github.com> Agent-Logs-Url: https://github.com/tomboulier/oxytcmri-dti-processing/sessions/3a05aa71-12b5-4555-8c93-5eaf8e8575e9 * fix: update remaining c3d STAPLE log message to picsl-c3d Convert3D Co-authored-by: tomboulier <14161960+tomboulier@users.noreply.github.com> Agent-Logs-Url: https://github.com/tomboulier/oxytcmri-dti-processing/sessions/3a05aa71-12b5-4555-8c93-5eaf8e8575e9 * Update oxytcmri/interface/mri/staple_segmenter.py * fix: simplify redundant picsl-c3d Convert3D wording Co-authored-by: tomboulier <14161960+tomboulier@users.noreply.github.com> Agent-Logs-Url: https://github.com/tomboulier/oxytcmri-dti-processing/sessions/f4f6fb26-bca3-4f32-afc2-b01ebec4c101 * Update oxytcmri/interface/mri/staple_segmenter.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update oxytcmri/interface/mri/staple_segmenter.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tomboulier <14161960+tomboulier@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|



Summary
c3dsubprocess dependency withpicsl-c3dwget/sudoinstall path from themakefilepicsl-c3d==1.4.6.0and refresh the lockfileVerification
uv sync --group dev./.venv/bin/pytest -q75 passedSummary by CodeRabbit