Add debugger updates and improve development tooling#40
Add debugger updates and improve development tooling#40
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a Python-driven bridge to MAD-NG’s interactive debugger and modernises the project’s development/testing workflow (pytest + uv + coverage), with accompanying documentation and CI updates.
Changes:
- Add a debugger bridge that can enter
MAD.dbg()from Python (MAD.breakpoint()/MAD.pydbg()) and expose MAD-side aliases (python_breakpoint,breakpoint,pydbg). - Migrate the test suite from
unittesttopytest, expanding debugger-focused tests and improving temp-file handling via pytest fixtures. - Improve development tooling/ops: add uv-based CI, pytest/coverage configuration, pre-commit + ruff + cspell integration, and Codecov reporting.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pymadng/madp_pymad.py | Implements debugger entry/command handling, improves shutdown, and updates recv_and_exec env handling. |
| src/pymadng/madp_object.py | Exposes MAD.breakpoint() / MAD.pydbg() and updates recv_and_exec context defaults. |
| src/pymadng/init.py | Bumps package version to 0.10.0. |
| tests/test_debug.py | Adds extensive debugger bridge coverage and improves log handling via tmp_path. |
| tests/test_examples_behaviour.py | Adds behavioural tests for close semantics, fork isolation, and multi-assignment. |
| tests/test_object_wrapping.py | Converts to pytest, adds coverage for deepcopy warnings and invalid indexing type. |
| tests/test_numeric_types.py | Converts to pytest, parametrises numeric edge cases and matrix send/recv tests. |
| tests/test_misc_types.py | Converts to pytest, parametrises range/monomial/TPSA tests. |
| tests/test_io_and_loading.py | Converts to pytest, uses tmp_path for loadfile tests. |
| tests/test_communication.py | Converts to pytest and parameterises string send tests. |
| tests/inputs/example.log | Updates expected debug log content to match current MAD version output. |
| pyproject.toml | Adds dev/test extras, pytest + coverage configuration. |
| .github/workflows/test-pymadng.yml | Switches CI to uv + pytest-cov and uploads coverage to Codecov. |
| .github/workflows/test-publish.yaml | Updates the published-package test workflow to run pytest. |
| .pre-commit-config.yaml | Adds ruff + cspell hooks. |
| cspell.config.yaml | Adds spell-check configuration for docs/README/changelog. |
| docs/source/.md, docs/source/.rst | Documents debugger usage, communication patterns, and updated contributor workflow. |
| README.md | Adds Codecov badge and clarifies early usage patterns. |
| CHANGELOG.md | Adds 0.10.0 release notes focused on the debugger bridge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while True: | ||
| command = input_stream.readline() | ||
| if command == "": | ||
| if self.process.poll() is not None: | ||
| raise RuntimeError("MAD debugger terminated the subprocess") | ||
| raise EOFError("Reached EOF while the MAD debugger was active") |
There was a problem hiding this comment.
In _run_interactive_debugger, hitting EOF on input_stream currently raises RuntimeError if the MAD subprocess has terminated, and EOFError otherwise. This can be flaky/incorrect for finite streams (e.g., StringIO) after sending a q command: the process may terminate slightly after the read timeout, causing the next readline() to hit EOF and raise instead of treating termination as a normal debugger exit. Consider, on EOF, polling _read_debugger_state() for a short grace period and, if the subprocess terminates, calling close() and returning cleanly (similar to the scripted path) rather than raising.
| if self.process.poll() is None: # If process is still running | ||
| self.send(f"{self.py_name}:__fin()") # Tell the mad side to finish | ||
| open_pipe = select.select([self.mad_read_stream], [], [], 5) # Shorter timeout | ||
| if open_pipe[0]: | ||
| # Wait for the mad side to finish | ||
| close_msg = self.recv("closing") | ||
| if close_msg != "<closing pipe>": | ||
| logging.warning( | ||
| f"Unexpected message received: {close_msg}, MAD-NG may not have completed properly" | ||
| ) | ||
| try: | ||
| self.send(f"{self.py_name}:__fin()") # Tell the mad side to finish | ||
| open_pipe = select.select([self.mad_read_stream], [], [], 5) # Shorter timeout | ||
| if open_pipe[0]: | ||
| # Wait for the mad side to finish | ||
| close_msg = self.recv("closing") | ||
| if close_msg != "<closing pipe>": | ||
| logging.warning( | ||
| f"Unexpected message received: {close_msg}, MAD-NG may not have completed properly" | ||
| ) | ||
| except BrokenPipeError: |
There was a problem hiding this comment.
close() now catches a BrokenPipeError raised during shutdown, but recv() can also return a BrokenPipeError instance as a sentinel when it reads an empty type tag (type_fun[""]["recv"] = BrokenPipeError). In that case close_msg = self.recv("closing") won't be caught by the exception handler and will fall through to the warning branch. Consider explicitly handling isinstance(close_msg, BrokenPipeError) (treat as already-closed) to avoid logging misleading warnings during normal/expected teardown paths (e.g., debugger quit).
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
No description provided.