Print the reminder for the illegal memory error in the AutoBatchSize under tf #4283
Conversation
… inherited class under tf
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4283 +/- ##
=======================================
Coverage 84.29% 84.30%
=======================================
Files 553 553
Lines 51820 51827 +7
Branches 3052 3052
=======================================
+ Hits 43683 43692 +9
+ Misses 7177 7176 -1
+ Partials 960 959 -1 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (1)
deepmd/tf/utils/batch_size.py (1)
22-31: Simplify the conditional logic structureWhile the implementation correctly moves the TF-specific memory error handling to this class, we can simplify the nested conditions.
Consider this cleaner structure:
def __init__(self, initial_batch_size: int = 1024, factor: float = 2.0) -> None: super().__init__(initial_batch_size, factor) DP_INFER_BATCH_SIZE = int(os.environ.get("DP_INFER_BATCH_SIZE", 0)) - if not DP_INFER_BATCH_SIZE > 0: - if self.is_gpu_available(): - log.info( + if not DP_INFER_BATCH_SIZE > 0 and self.is_gpu_available(): + log.info(🧰 Tools
🪛 Ruff
25-26: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/tf/utils/batch_size.py(2 hunks)
🧰 Additional context used
🪛 Ruff
deepmd/tf/utils/batch_size.py
25-26: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (2)
deepmd/tf/utils/batch_size.py (2)
Line range hint 2-18: LGTM: Import changes align with PR objectives
The new imports support the relocation of the logging functionality from the base class to this TF-specific implementation.
🧰 Tools
🪛 Ruff
25-26: Use a single if statement instead of nested if statements
(SIM102)
22-32: Implementation successfully meets PR objectives
The changes effectively:
- Move the illegal memory reminder to the TF-specific class
- Only show the message in relevant contexts (GPU with TF)
- Maintain helpful guidance for users
🧰 Tools
🪛 Ruff
25-26: Use a single if statement instead of nested if statements
(SIM102)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deepmd/tf/utils/batch_size.py (1)
22-30: LGTM with a minor suggestion for robustnessThe constructor implementation successfully moves the TensorFlow-specific memory error reminder to the appropriate class. The message is clear and actionable.
Consider adding error handling for invalid environment variable values:
- DP_INFER_BATCH_SIZE = int(os.environ.get("DP_INFER_BATCH_SIZE", 0)) + try: + DP_INFER_BATCH_SIZE = int(os.environ.get("DP_INFER_BATCH_SIZE", 0)) + except ValueError: + log.warning("Invalid DP_INFER_BATCH_SIZE value, defaulting to 0") + DP_INFER_BATCH_SIZE = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
deepmd/tf/utils/batch_size.py(2 hunks)deepmd/utils/batch_size.py(0 hunks)
💤 Files with no reviewable changes (1)
- deepmd/utils/batch_size.py
🔇 Additional comments (2)
deepmd/tf/utils/batch_size.py (2)
16-18: LGTM: Clean import addition
The log import is properly placed and follows the existing import style.
22-30: Verify corresponding removal in base class
Let's confirm that the reminder message was properly removed from the base class.
✅ Verification successful
The verification results show that:
- The reminder message is not present in the base class (
deepmd/utils/batch_size.py) - The reminder message is only present in the TF-specific class (
deepmd/tf/utils/batch_size.py)
Let me verify if this was indeed a move from base class to TF-specific class by checking the git history:
Verified: Reminder message correctly moved from base to TF-specific class
The git history confirms that the reminder message was properly moved from the base class (AutoBatchSize in deepmd/utils/batch_size.py) to the TF-specific class in deepmd/tf/utils/batch_size.py. This aligns with the PR's objective of making the TensorFlow-specific error message only appear in the TF implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the reminder message was removed from the base class
# and only exists in the TF-specific class
# Check for the reminder message in the base class
echo "Checking base class (should not find the message):"
rg "an illegal memory access was encountered" "deepmd/utils/batch_size.py"
# Check for the reminder message in the TF class (should find the message)
echo -e "\nChecking TF class (should find the message):"
rg "an illegal memory access was encountered" "deepmd/tf/utils/batch_size.py"
Length of output: 497
Script:
#!/bin/bash
# Check git history for the reminder message to confirm it was moved
git log -p | rg -B 5 -A 5 "an illegal memory access was encountered"
Length of output: 3149
#3822 added a reminder for the illegal memory error. However, this reminder is only needed for tf. This PR moves the illegal memory reminder from base class AutoBatchSize to the inherited class under tf.
Summary by CodeRabbit
AutoBatchSizeclass to initialize batch size from an environment variable, improving user guidance on memory management with TensorFlow.