Log to logger instead of discord. #648
Conversation
brettelliot
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary by Korbit AI
Code Execution Comments
- Address the mismatch between the warning message and actual behavior in rebalance logic for correct execution and transparency.
Files scanned
| File Path | Reviewed |
|---|---|
| lumibot/example_strategies/drift_rebalancer.py | ✅ |
| lumibot/components/drift_rebalancer_logic.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
| if all([abs(weight) < self.drift_threshold for weight in target_weights.values()]): | ||
| self.strategy.logger.warning( | ||
| f"All target weights are less than the drift_threshold: {self.drift_threshold}. " | ||
| f"No rebalance will be triggered." | ||
| ) |
There was a problem hiding this comment.
Misleading Warning Without Action
Tell me more
What is the issue?
The warning message states that no rebalance will be triggered, but the code continues execution without preventing the rebalance. This creates a mismatch between the warning message and actual behavior.
Why this matters
Users may be misled by the warning message, expecting no rebalance to occur when in fact the code will attempt to rebalance anyway. This could lead to unexpected portfolio adjustments.
| self.strategy.logger.warning( | ||
| f"All target weights are less than the drift_threshold: {self.drift_threshold}. " | ||
| f"No rebalance will be triggered." | ||
| ) |
There was a problem hiding this comment.
Include individual target weights in the warning log.
Tell me more
In the calculate() method, consider including individual target weights in the warning log to give better context on why a rebalance won't be triggered.
Description by Korbit AI
What change is being made?
Refactor the logging mechanism to use the logger exclusively instead of directly logging messages to Discord.
Why are these changes being made?
These changes enhance modularity and maintainability by consolidating logging through a standard logger, reducing reliance on platform-specific logging (Discord). This refactor simplifies the code by removing redundant message formatting for broadcast and allows easier future adjustments to logging behavior or destinations without impacting individual logic flows.