Skip to content

Debug - #4

Closed
krassowski wants to merge 28 commits into
mainfrom
debug
Closed

Debug#4
krassowski wants to merge 28 commits into
mainfrom
debug

Conversation

@krassowski

Copy link
Copy Markdown
Owner

No description provided.

@krassowski
krassowski requested a review from CopilotMarch 31, 2025 11:47

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the notebook export functionality and test coverage for capturing various output types, while also updating dependency management and refining output history and display handling.

  • Updated notebook export logic to capture errors, display outputs, and streams.
  • Introduced a new test (test_notebook_export_json_with_output) to verify proper output capture.
  • Added new dependencies and refined history, display, and tee functionality.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
tests/test_magic.pyAdded new test for notebook export with output capture and removed extra blank lines.
pyproject.tomlAdded nbclient and ipykernel dependencies to support new notebook export tests.
IPython/core/magics/basic.pyUpdated export logic to iterate over outputs and append error outputs if present.
IPython/core/interactiveshell.pyRefactored exception formatting and introduced capturing tee for stdout/stderr.
IPython/core/history.pyAdded HistoryOutput dataclass and updated reset to clear outputs and exceptions.
IPython/core/displaypub.pyModified publish logic to record HistoryOutput and maintain publishing state.
IPython/core/displayhook.pyAdded active flag to manage display hook state.
IPython/core/display_trap.pyAdded a property to check if the display trap is active.

Comment threadtests/test_magic.py
Comment on lines +956 to +957
print(f"\n{_ip.history_manager.outputs}\n")

CopilotAIMar 31, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This print statement appears to be a leftover debugging statement. Consider removing it to keep test output clean.

Suggested change
print(f"\n{_ip.history_manager.outputs}\n")

Copilot uses AI. Check for mistakes.
Comment threadtests/test_magic.py
with TemporaryDirectory() as td:
outfile = os.path.join(td, "nb.ipynb")
_ip.run_cell(f"%notebook {outfile}", store_history=True)
sleep(2)

CopilotAIMar 31, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Relying on a fixed sleep duration can lead to flaky tests. Consider replacing it with a dynamic wait or polling mechanism to ensure reliable synchronization.

Suggested change
sleep(2)
timeout=10
interval=0.1
elapsed=0
whileelapsed<timeout:
ifos.path.exists(outfile) andos.access(outfile, os.R_OK):
break
sleep(interval)
elapsed+=interval

Copilot uses AI. Check for mistakes.
@krassowski

Copy link
Copy Markdown
OwnerAuthor

Commenting out CapturingTee part makes the test (macos-13, 3.13) pass.

Comment threadIPython/core/interactiveshell.py Outdated
@krassowski
krassowski requested a review from CopilotMarch 31, 2025 18:58

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR, titled "Debug", updates tests and core IPython modules to improve notebook export functionality and error handling.

  • Added a new test (test_notebook_export_json_with_output) to verify that notebook export captures outputs, errors, and display outputs correctly.
  • Updated dependency declarations in pyproject.toml to include nbclient and ipykernel.
  • Enhanced IPython internals including history outputs, display publishing/trapping, and traceback capture mechanisms.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
tests/test_magic.pyAdded a new test for notebook export and modified test structure
pyproject.tomlUpdated test dependencies
IPython/core/magics/basic.pyUpdated notebook magic to include output and exception handling
IPython/core/interactiveshell.pyIntroduced a _tee context manager and adjusted exception formatting
IPython/core/history.pyIntroduced a HistoryOutput dataclass; cleared history outputs on reset
IPython/core/displaypub.pyAdded publishing state tracking and output logging adjustments
IPython/core/displayhook.pyAdded an active state for displayhook and adjusted output logging
IPython/core/display_trap.pyAdded an is_active property for display trap
Comments suppressed due to low confidence (1)

IPython/core/interactiveshell.py:3049

  • Ensure that 'data' is always a string to prevent potential type concatenation issues; consider casting 'data' to str if necessary.
output_stream.bundle["stream"] += data

Comment threadtests/test_magic.py
Comment on lines +956 to +957
print(f"\n{_ip.history_manager.outputs}\n")

CopilotAIMar 31, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider removing the debug print statement to avoid cluttering the test output in production.

Suggested change
print(f"\n{_ip.history_manager.outputs}\n")

Copilot uses AI. Check for mistakes.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@krassowski@Darshan808