Problem: output artifact gets expired easily #1671
Conversation
record in log
mmsqe
requested review from
calvinaco and
thomas-nguy
and removed request for
a team
WalkthroughThe changes introduce a new Changes
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 @@
## main #1671 +/- ##
===========================================
- Coverage 35.27% 16.84% -18.43%
===========================================
Files 123 72 -51
Lines 11752 6161 -5591
===========================================
- Hits 4145 1038 -3107
+ Misses 7193 5000 -2193
+ Partials 414 123 -291 |
![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 (3)
testground/benchmark/benchmark/stats.py (3)
60-61: Refactor duplicate print statements to improve maintainability.The identical format in both print statements creates unnecessary duplication. Consider extracting the format into a single string template.
- print("block", i, txs, timestamp, tps) - print("block", i, txs, timestamp, tps, file=fp) + stats_line = f"block {i} {txs} {timestamp} {tps}" + print(stats_line) + print(stats_line, file=fp)
Line range hint
33-63: Consider implementing error handling and memory optimization.The function has several areas that could be improved:
- No error handling for file operations or RPC calls
- Keeps all blocks in memory which could be problematic for large ranges
- The function name undersells its capabilities
Consider these improvements:
- Add try-except blocks around RPC calls and file operations
- Implement periodic file flushing to ensure data persistence
- Consider renaming to
analyze_block_statsor similar to better reflect its TPS analysis capabilityWould you like me to provide a detailed implementation of these improvements?
Line range hint
33-63: Address artifact expiration by implementing proper file handling.This change adds console output but doesn't address the core issue of artifact expiration. The function should ensure data persistence through proper file handling.
Consider these changes to address artifact expiration:
def dump_block_stats( fp, eth=True, json_rpc=LOCAL_JSON_RPC, rpc=LOCAL_RPC, start: int = 2, end: int = None, ): + # Ensure file handle is in write mode and buffering is enabled + if not fp.writable(): + raise ValueError("File handle must be writable") + tps_list = [] if end is None: end = block_height(rpc) blocks = [] # skip block 1 whose timestamp is not accurate for i in range(start, end + 1): if eth: timestamp, txs = get_block_info_eth(i, json_rpc) else: timestamp, txs = get_block_info_cosmos(i, rpc) blocks.append((txs, timestamp)) tps = calculate_tps(blocks[-TPS_WINDOW:]) tps_list.append(tps) print("block", i, txs, timestamp, tps) print("block", i, txs, timestamp, tps, file=fp) + # Flush every N blocks to ensure data persistence + if i % 100 == 0: + fp.flush() tps_list.sort(reverse=True) print("top_tps", tps_list[:5], file=fp) + fp.flush() # Final flush to ensure all data is written
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
testground/benchmark/benchmark/utils.py (2)
212-221: LGTM! Consider adding essential file-like object methods.The
Teeclass implementation looks good for the basic use case of duplicating output. However, it should implement additional essential methods to fully comply with the file-like object protocol.Add these essential methods:
class Tee: def __init__(self, f1, f2): self.f1 = f1 self.f2 = f2 def write(self, s) -> int: s1 = self.f1.write(s) s2 = self.f2.write(s) assert s1 == s2 return s1 + + def flush(self): + self.f1.flush() + self.f2.flush() + + def close(self): + self.f1.close() + self.f2.close()
212-221: Consider these improvements for better robustness.The current implementation could be enhanced with better type safety and error handling.
Consider these improvements:
+from typing import TextIO + class Tee: - def __init__(self, f1, f2): + def __init__(self, f1: TextIO, f2: TextIO): self.f1 = f1 self.f2 = f2 def write(self, s) -> int: - s1 = self.f1.write(s) - s2 = self.f2.write(s) - assert s1 == s2 - return s1 + try: + s1 = self.f1.write(s) + s2 = self.f2.write(s) + if s1 != s2: + raise IOError(f"Write sizes differ: {s1} != {s2}") + return s1 + except Exception as e: + raise IOError(f"Write failed: {str(e)}")This version:
- Adds type hints for better IDE support
- Replaces assertion with proper error handling
- Wraps writes in try-except to handle IO errors gracefully
testground/benchmark/benchmark/stateless.py (1)
306-306: Consider enhancing error handlingWhile the current implementation is good, consider adding error handling for potential I/O issues with the
Teewriter. This would make the test infrastructure more robust.Example scenarios to handle:
- IOError when writing to either destination
- Partial writes
- Resource cleanup on errors
- dump_block_stats(Tee(logfile, sys.stdout)) + try: + tee = Tee(logfile, sys.stdout) + dump_block_stats(tee) + except IOError as e: + print(f"Warning: Failed to write block stats: {e}", file=sys.stderr) + # Attempt to write to individual streams as fallback + try: + dump_block_stats(logfile) + except IOError: + try: + dump_block_stats(sys.stdout) + except IOError: + print("Error: Failed to write block stats to any output", file=sys.stderr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
testground/benchmark/benchmark/stateless.py(2 hunks)testground/benchmark/benchmark/utils.py(1 hunks)
🔇 Additional comments (2)
testground/benchmark/benchmark/stateless.py (2)
31-31: LGTM: Clean import addition
The import of Tee is properly placed with related utility imports.
306-306: LGTM: Enhanced logging visibility
Good improvement that writes block stats to both the log file and console. This ensures:
- Block statistics are preserved in output artifacts
- Real-time visibility in console output for monitoring
- Data availability even if artifacts expire
This change effectively addresses the PR objective of handling output artifact expiration.
nice, can tee 2 outputs at the same time |
45aaff0
record in log
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Teeclass to enable simultaneous logging to both a file and the console.do_runfunction for improved output handling.Bug Fixes
Documentation
Teeclass.