Conversation
📝 WalkthroughWalkthroughBump package release from 0.5.5 → 0.6.0 across metadata, docs, and dependency, update CLI help text formatting, and change HMC QC code: memory allocation increase, modified HMC_derivatives return signature, updated caller uses, and added explicit deletions for intermediates. Changes
Sequence Diagram(s)(Skipped — changes do not meet the multi-component sequence diagram criteria.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
🧹 Nitpick comments (2)
rabies/preprocess_pkg/hmc.py (2)
61-63: Avoid baking the 20x QC memory bump into the workflow.
opts.scale_min_memoryis already the scheduler-facing scaling input, so another unconditional*20makes this node's footprint cluster-specific and hard to tune. Please move this multiplier into config/CLI instead of hard-coding it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabies/preprocess_pkg/hmc.py` around lines 61 - 63, The hmc_qc_node currently hard-codes a 20x memory multiplier (mem_gb=1.1*opts.scale_min_memory*20), which should be moved to configuration/CLI; remove the unconditional *20 in the hmc_qc_node creation so mem_gb uses 1.1*opts.scale_min_memory, add a new config/CLI option (e.g. opts.hmc_qc_memory_multiplier or opts.scale_memory_multiplier) and apply that multiplier when the user sets it (mem_gb = 1.1 * opts.scale_min_memory * opts.hmc_qc_memory_multiplier), and update the CLI/config parsing and documentation to expose and default this multiplier rather than baking it into the HMC_QC/hmc_qc_node instantiation.
275-277: Close the Matplotlib figure after writing it.
plot_motion_QC()creates the figure throughmatplotlib.pyplot, so it stays registered until explicitly closed. In this already RAM-sensitive node, releasing it right aftersavefig()will help keep worker memory flatter.Proposed fix
print('Creating QC figure...') fig = plot_motion_QC(derivatives_dict, self.inputs.ref_file, plot_R2=False) fig.savefig(figure_path, bbox_inches='tight') + import matplotlib.pyplot as plt + plt.close(fig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rabies/preprocess_pkg/hmc.py` around lines 275 - 277, The Matplotlib figure created by plot_motion_QC remains registered after savefig causing memory to accumulate; after fig.savefig(figure_path, bbox_inches='tight') call, close and release the figure (e.g., call matplotlib.pyplot.close(fig) or fig.close()) to free memory; ensure matplotlib.pyplot is imported in the module if not present and add the close call immediately after the savefig line in the same block where plot_motion_QC is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rabies/preprocess_pkg/hmc.py`:
- Around line 61-63: The hmc_qc_node currently hard-codes a 20x memory
multiplier (mem_gb=1.1*opts.scale_min_memory*20), which should be moved to
configuration/CLI; remove the unconditional *20 in the hmc_qc_node creation so
mem_gb uses 1.1*opts.scale_min_memory, add a new config/CLI option (e.g.
opts.hmc_qc_memory_multiplier or opts.scale_memory_multiplier) and apply that
multiplier when the user sets it (mem_gb = 1.1 * opts.scale_min_memory *
opts.hmc_qc_memory_multiplier), and update the CLI/config parsing and
documentation to expose and default this multiplier rather than baking it into
the HMC_QC/hmc_qc_node instantiation.
- Around line 275-277: The Matplotlib figure created by plot_motion_QC remains
registered after savefig causing memory to accumulate; after
fig.savefig(figure_path, bbox_inches='tight') call, close and release the figure
(e.g., call matplotlib.pyplot.close(fig) or fig.close()) to free memory; ensure
matplotlib.pyplot is imported in the module if not present and add the close
call immediately after the savefig line in the same block where plot_motion_QC
is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e309d8d3-0f8a-4925-bfbe-6fc05b1eb4d5
📒 Files selected for processing (2)
rabies/parser.pyrabies/preprocess_pkg/hmc.py
✅ Files skipped from review due to trivial changes (1)
- rabies/parser.py
Summary by CodeRabbit
Chores
Documentation
User-facing fixes
Improvements