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.
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 |
|
Incorrectly linked to this in my PR reopening |
- _get_sample_mac now returns None when no processes found, matching Linux semantics where collect_sample returns None for empty sessions - Simplified _add_pid_to_sample_from_line_mac: inverted check, no return - Removed skipempty flag entirely - no longer needed since monitor_process already handles None samples correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
return None for empty Mac samples, remove skipempty flag
|
CI runs look to be pretty stable now as well! |
asmacdo
left a comment
asmacdo
left a comment
There was a problem hiding this comment.
I'll have another look tomorrow morning with fresh eyes. Just a couple nitpicks at this point, I think this is almost ready.
Thanks for all this effort @CodyCBakerPhD <3
src/con_duct/duct_main.py
Outdated
| """ | ||
| try: | ||
| return os.getsid(pid) | ||
| except Exception as exc: |
There was a problem hiding this comment.
I think we can tighten this up a bit
| except Exception as exc: | |
| except OSError as exc: |
There was a problem hiding this comment.
I was trying to be pretty broad, but if you want to be specific, it is a ProcessLookupError that is the most common. IDK if there are any others aside from improper input
There was a problem hiding this comment.
(which I have now adjusted)
| os: | ||
| # - macos-12 | ||
| # - macos-latest | ||
| - macos-latest |
There was a problem hiding this comment.
this is an arm one, and I see some warning to be issued on intel below. Should we may be add
| - macos-latest | |
| - macos-latest # arm64 | |
| - macos-latest-large # intel |
to test on both?
There was a problem hiding this comment.
I don't believe our organization use large runners unless you upgraded the plan
macos-15-intel would be our next bet but I would just test in a follow-up since this has gone on long enough
|
|
||
|
|
4 participants
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)