Rename internal plotting functions #776
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes focus on renaming plotting-related functions in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
=======================================
Coverage 97.65% 97.65%
=======================================
Files 32 32
Lines 1451 1451
=======================================
Hits 1417 1417
Misses 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
tests/test_singlenodeexecutor_plot_dependency.py (6)
125-130: See earlier comment – identical change.
142-147: See earlier comment – identical change.
170-175: See earlier comment – identical change.
215-220: See earlier comment – identical change.
241-246: See earlier comment – identical change.
284-289: See earlier comment – identical change.tests/test_fluxjobexecutor_plot.py (3)
103-108: See previous call – same change.
129-134: See previous call – same change.
172-177: See previous call – same change.
🧹 Nitpick comments (1)
executorlib/standalone/plot.py (1)
8-20: Function renamed; consider updating__all__for public API clarityRenaming is fine, but if this module intends to expose a stable public surface, an
__all__list would make the exported names explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/standalone/plot.py(3 hunks)executorlib/task_scheduler/interactive/dependency.py(3 hunks)tests/test_fluxjobexecutor_plot.py(5 hunks)tests/test_singlenodeexecutor_plot_dependency.py(8 hunks)tests/test_testclusterexecutor.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unittest_win
- GitHub Check: notebooks
- GitHub Check: notebooks_integration
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_old
- GitHub Check: unittest_slurm_mpich
🔇 Additional comments (11)
tests/test_testclusterexecutor.py (2)
7-7: Import correctly updated to new helper nameThe renamed symbol
generate_nodes_and_edges_for_plottingis imported from the expected location and keeps the test in sync with the plot helper API.
86-91: Call site aligns with new API – no further actionThe call was renamed consistently and positional arguments remain correct (
task_hash_dict,future_hash_inverse_dict).tests/test_singlenodeexecutor_plot_dependency.py (2)
10-10: Updated import reflects helper rename
No issues – import path and symbol are correct.
64-69: First call site updated – counts & semantics unchanged
The helper is invoked with the same arguments; nothing else to flag.tests/test_fluxjobexecutor_plot.py (2)
6-6: Import brought in line with renamed helper
No further issues detected.
58-63: Helper call renamed – arguments correct.executorlib/task_scheduler/interactive/dependency.py (3)
147-153: Hash generation now uses new helper – matches rename
Call arguments are unchanged; return value feeds directly into_future_hash_dictas before.
208-218: Graph-building pipeline updated end-to-end
generate_nodes_and_edges_for_plottingand theplot_dependency_graphwrapper are chained correctly, preserving previous behaviour.
15-18: No leftover helper references found after renaming
The grep results only match networkx’s.draw()calls inexecutorlib/standalone/plot.py(lines 158 & 162). There are no occurrences of the old names (generate_nodes_and_edges_for_plotting,generate_task_hash_for_plotting, orplot_dependency_graph). Everything looks consistent.executorlib/standalone/plot.py (2)
84-86: Consistent naming – keep docstring tags in sync
Docstring reflects the new name; looks good.
137-139: Top-level draw helper renamed; no logic changes
Implementation untouched, rename is consistent.
Summary by CodeRabbit
Refactor
Tests