🎨 Stremline error handling#834
Conversation
denialhaag
force-pushed
the
streamline-error-handling
branch
from
32d30da to
7b26e57
Compare
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughError messages in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/bench/output.py (1)
157-170:⚠️ Potential issue | 🟡 MinorMissing
# pragma: no coveron the QPY stream error handler.The QASM stream handler (line 157) is annotated
# pragma: no cover - unforeseen I/O, but the structurally identical QPY stream handler (line 168) is not. Either both should be marked (if neither path is reachable in tests) or neither should be.🛠️ Proposed fix
try: dump_qpy(_attach_metadata(qc, header), destination) - except Exception as exc: + except Exception as exc: # pragma: no cover - unforeseen I/O msg = "Failed to write QPY stream." raise MQTBenchExporterError(msg) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/bench/output.py` around lines 157 - 170, The QPY exception handler should mirror the QASM handler's coverage pragma: in the block handling OutputFormat.QPY (check the fmt == OutputFormat.QPY branch) add the same "# pragma: no cover - unforeseen I/O" annotation to the "except Exception as exc:" line that wraps dump_qpy(_attach_metadata(qc, header), destination), so the raised MQTBenchExporterError from that except is excluded from coverage reports (references: OutputFormat.QPY, dump_qpy, _attach_metadata, MQTBenchExporterError, TextIOBase).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mqt/bench/output.py`:
- Around line 157-159: The MQTBenchExporterError raised by the various write
handlers (they raise MQTBenchExporterError(msg) from exc) loses actionable
detail when save_circuit catches it and only does print(e); update save_circuit
to also surface the original cause by printing or logging e.__cause__ (e.g.,
print(e, "Cause:", e.__cause__ or str(e.__cause__)), or include str(e.__cause__)
when formatting the message) so callers see the original exception; make this
change in the save_circuit exception handling block that catches
MQTBenchExporterError (affects the save_circuit path that calls the write_*
handlers and write_circuit).
---
Outside diff comments:
In `@src/mqt/bench/output.py`:
- Around line 157-170: The QPY exception handler should mirror the QASM
handler's coverage pragma: in the block handling OutputFormat.QPY (check the fmt
== OutputFormat.QPY branch) add the same "# pragma: no cover - unforeseen I/O"
annotation to the "except Exception as exc:" line that wraps
dump_qpy(_attach_metadata(qc, header), destination), so the raised
MQTBenchExporterError from that except is excluded from coverage reports
(references: OutputFormat.QPY, dump_qpy, _attach_metadata,
MQTBenchExporterError, TextIOBase).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mqt/bench/output.py`:
- Around line 181-183: The raised MQTBenchExporterError currently omits the
original exception text, breaking tests that expect the cause (e.g., "disk
full"); update the except handlers that raise MQTBenchExporterError to include
the original exception message (exc) in the new message (e.g., f"Failed to write
{fmt.value.upper()} file to {destination}: {exc}"), and apply the same change to
the similar handlers referenced for stream and QPY paths so the original error
cause is preserved in the MQTBenchExporterError message.
---
Duplicate comments:
In `@src/mqt/bench/output.py`:
- Around line 222-223: In save_circuit's exception handler (catching
MQTBenchExporterError in output.py) replace the silent print(e) with code that
surfaces the original cause: log or print both the MQTBenchExporterError and its
__cause__ (or the full traceback) so the underlying OSError/encoding error is
visible; specifically update the except block referencing MQTBenchExporterError
to include e.__cause__ (or traceback.format_exc()) in the message or
re-raise/raise a new error using "from e.__cause__" so the root cause is not
discarded.
| except Exception as exc: | ||
| msg = f"Failed to write {fmt.value.upper()} file to {destination}. (Original error: {exc})" | ||
| msg = f"Failed to write {fmt.value.upper()} file to {destination}." | ||
| raise MQTBenchExporterError(msg) from exc |
There was a problem hiding this comment.
Removing {exc} from the error message breaks test_write_circuit_io_error.
test_write_circuit_io_error in tests/test_bench.py (line 787) asserts:
assert "disk full" in msg.lower() # msg = str(exc.value)str(exc.value) is the MQTBenchExporterError message string only — Python does not include __cause__ in str(). With the current message f"Failed to write {fmt.value.upper()} file to {destination}.", the string does not contain "disk full", so the assertion fails.
The same applies to lines 158, 169, and 190 (stream and QPY paths), though those are not exercised by this particular test case.
Either update the test to drop the "disk full" assertion and rely solely on the generic message check at line 786, or re-embed the cause in the message:
🛠️ Option A — Update the test (aligned with PR intent)
msg = str(exc.value)
assert "failed to write qasm2 file" in msg.lower()
-assert "disk full" in msg.lower()🛠️ Option B — Re-embed the exception in the message
- msg = f"Failed to write {fmt.value.upper()} file to {destination}."
+ msg = f"Failed to write {fmt.value.upper()} file to {destination}: {exc}."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/bench/output.py` around lines 181 - 183, The raised
MQTBenchExporterError currently omits the original exception text, breaking
tests that expect the cause (e.g., "disk full"); update the except handlers that
raise MQTBenchExporterError to include the original exception message (exc) in
the new message (e.g., f"Failed to write {fmt.value.upper()} file to
{destination}: {exc}"), and apply the same change to the similar handlers
referenced for stream and QPY paths so the original error cause is preserved in
the MQTBenchExporterError message.
|
Closing this as CoreRabbit seems not to like the overall idea. |
1 participant
Description
This PR streamlines the error handling after an oversight in #833.
Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.I have added migration instructions to the upgrade guide (if needed).