-
Notifications
You must be signed in to change notification settings - Fork 3
Cache: Support classes without __name__ attribute #865
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.
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
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 2 minutes and 1 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)
WalkthroughReplaced direct use of fn.name with a new _get_function_name(fn) helper in serialize_funct to robustly obtain function names; added a test that submits a callable class instance to validate caching behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_singlenodeexecutor_cache.py (1)
20-23: LGTM - callable class for testing.The
AddClassimplementation correctly provides a callable instance without a__name__attribute, which is exactly what this PR aims to support.Optional: For PEP 8 compliance, add spaces around the
+operator on line 22:return a + b
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/executorlib/standalone/serialize.py(2 hunks)tests/test_singlenodeexecutor_cache.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_singlenodeexecutor_cache.py (1)
src/executorlib/executor/single.py (1)
SingleNodeExecutor(20-190)
⏰ 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). (20)
- GitHub Check: unittest_win
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_old
- GitHub Check: notebooks
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_mpich (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_flux_openmpi
- GitHub Check: notebooks_integration
- GitHub Check: unittest_flux_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (1)
src/executorlib/standalone/serialize.py (1)
78-78: Verify the helper implementation handles all cases correctly.The change from
fn.__name__to_get_function_name(fn=fn)correctly supports callables without the__name__attribute. However, ensure the helper function's fallback logic (parsing__str__()) produces consistent, valid names across different callable types.
| def _get_function_name(fn: Callable) -> str: | ||
| if hasattr(fn, "__name__"): | ||
| return fn.__name__ | ||
| elif hasattr(fn, "__str__"): | ||
| return fn.__str__().split()[0].split(".")[-1] | ||
| else: | ||
| return "fn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Strengthen the fallback logic for callable instances.
The __str__() parsing on line 108 is fragile and may produce inconsistent results:
- For instances without module qualification (e.g.,
<AddClass object at 0x...>),split(".")[-1]would return<AddClass(including the<character) - Custom
__str__()implementations could break this parsing entirely - The logic assumes a specific
__str__format that may not hold for all callables
Consider using type(fn).__name__ or fn.__class__.__name__ for callable instances, which directly returns the class name without fragile string parsing.
Apply this diff to improve robustness:
def _get_function_name(fn: Callable) -> str:
if hasattr(fn, "__name__"):
return fn.__name__
- elif hasattr(fn, "__str__"):
- return fn.__str__().split()[0].split(".")[-1]
+ elif hasattr(fn, "__class__"):
+ return fn.__class__.__name__
else:
return "fn"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_function_name(fn: Callable) -> str: | |
| if hasattr(fn, "__name__"): | |
| return fn.__name__ | |
| elif hasattr(fn, "__str__"): | |
| return fn.__str__().split()[0].split(".")[-1] | |
| else: | |
| return "fn" | |
| def _get_function_name(fn: Callable) -> str: | |
| if hasattr(fn, "__name__"): | |
| return fn.__name__ | |
| elif hasattr(fn, "__class__"): | |
| return fn.__class__.__name__ | |
| else: | |
| return "fn" |
🤖 Prompt for AI Agents
In src/executorlib/standalone/serialize.py around lines 104 to 110, the fallback
that parses fn.__str__() is fragile and can produce incorrect names (e.g.,
includes “<” or depends on custom __str__ formats); change the logic to return
fn.__name__ if present, otherwise return type(fn).__name__ (or
fn.__class__.__name__) for callable instances, and only fall back to the literal
"fn" if neither is available, keeping the returned value as a simple string.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
==========================================
+ Coverage 93.26% 93.27% +0.01%
==========================================
Files 38 38
Lines 1796 1800 +4
==========================================
+ Hits 1675 1679 +4
Misses 121 121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Example:
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.