Added Mac (M-series) support #351
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for macOS (M-series) to the con-duct tool, enabling process monitoring and resource tracking on Mac systems. The implementation introduces platform-specific process sampling logic to handle differences between Linux and macOS ps command behavior.
Key Changes:
- Added Mac-specific process sampling functions that handle session ID tracking differently than Linux
- Introduced a
--skipemptyflag (default True on macOS, False on Linux) to handle empty sample collection gracefully - Updated tests to skip Linux-specific behavior checks on other platforms
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/con_duct/duct_main.py | Core implementation of Mac support with platform detection, Mac-specific sampling functions (_get_sample_mac, _get_ps_lines_mac, _add_sample_from_line_mac), skipempty parameter support, and Intel Mac warning |
| src/con_duct/cli.py | Added --skipempty CLI argument with platform-specific defaults |
| test/test_cli.py | Added platform detection and marked Linux-specific tests to skip on other systems; fixed typo in test function name |
| test/duct_main/test_e2e.py | Added platform detection constant and commented-out experimental code |
| test/duct_main/test_aggregation.py | Updated test to pass skipempty parameter to update_from_sample |
| setup.cfg | Added MacOS classifier to project metadata |
| pyproject.toml | Added pytest marker configuration for flaky tests |
| .gitignore | Added .DS_Store exclusion for macOS |
| .github/workflows/test.yaml | Enabled macOS testing in CI workflow |
Comments suppressed due to low confidence (1)
test/duct_main/test_e2e.py:174
- This comment appears to contain commented-out code.
# if SYSTEM == "Darwin":
# skip_flag = " --skipempty"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _get_sample: Callable[[int], Sample] = _get_sample_per_system[SYSTEM] # type: ignore[assignment] | ||
|
|
||
|
|
There was a problem hiding this comment.
This code will raise a KeyError at module import time if the code is run on an unsupported system (e.g., Windows). Consider adding error handling or a default fallback to provide a clear error message indicating which systems are supported.
| _get_sample: Callable[[int], Sample] = _get_sample_per_system[SYSTEM] # type: ignore[assignment] | |
| def _get_sample(session_id: int) -> Sample: | |
| """Return a Sample for the given session_id, dispatching based on the current system. | |
| Raises a clear error on unsupported systems instead of failing with a KeyError | |
| at import time. | |
| """ | |
| try: | |
| backend = _get_sample_per_system[SYSTEM] | |
| except KeyError as exc: | |
| supported = ", ".join(sorted(_get_sample_per_system.keys())) | |
| raise RuntimeError( | |
| f"Unsupported system '{SYSTEM}'. Supported systems are: {supported}." | |
| ) from exc | |
| return backend(session_id) |
There was a problem hiding this comment.
Leaving for @asmacdo decision - the current pattern was based on Yariks original suggestion
There was a problem hiding this comment.
Personally, I have no issue with a KeyError right now. I'll add Windows support in the new year
CodyCBakerPhD
added
enhancement
|
CI is still a tad flaky on certain tests, but feels better than before A fully passing run: https://github.com/con/duct/actions/runs/20389066354/job/58595457605?pr=351 @asmacdo I leave it up to you to decide how/if to skip/retry those |
1 participant
Redo of #325 under the new design with minimal adjustments for Mac support
The tests as written run fine and reliably on my local device, but we'll see what the CI says
I did add a specific behavior for skipping samples during aggregation to pass a couple of tests, but I emphasize that the behavior of the tool on my system (when I run the same sleep command through terminal) appears to be working as intended, and the reason the samples fail to get captured is unknown (which appear to be the final samples captured in the process)