feat(windows): native Windows support — platform abstraction layer#946
feat(windows): native Windows support — platform abstraction layer#946lkomali wants to merge 3 commits into
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@a5313a619a5c5dc34bf2e758bfeb2776240989e7Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@a5313a619a5c5dc34bf2e758bfeb2776240989e7Last updated for commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors OS detection to import-time constants; adds Windows asyncio event-loop policy setup and Windows-specific synchronous signal handlers that bridge into asyncio; implements cross-platform ZMQ address helper (IPC vs deterministic TCP); narrows process typing; adjusts command parsing and broad test-suite platform handling. ChangesWindows Cross-Platform Support
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 3
🧹 Nitpick comments (1)
tests/unit/zmq/test_dual_bind.py (1)
119-119: ⚡ Quick winUse raw strings for regex patterns to fix RUF043 warnings.
The regex patterns contain metacharacters (square brackets) but are not raw strings. Ruff warns because the pattern should explicitly indicate it's a regex by using a raw string prefix.
🔧 Proposed fix
- with pytest.raises(ValueError, match="[Pp]ath is required"): + with pytest.raises(ValueError, match=r"[Pp]ath is required"):Apply the same change at line 629:
- with pytest.raises(ValueError, match="[Pp]ath is required"): + with pytest.raises(ValueError, match=r"[Pp]ath is required"):Also applies to: 629-629
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/zmq/test_dual_bind.py` at line 119, The pytest.raises calls that use match="[Pp]ath is required" should use a raw string to avoid regex escaping warnings: update the match argument in the pytest.raises(ValueError, match=...) at the occurrence referencing ValueError (line with match "[Pp]ath is required") and the other occurrence at the later test (around the second occurrence) to use a raw string literal (r"...") so the pattern is r"[Pp]ath is required"; locate these in tests/unit/zmq/test_dual_bind.py where pytest.raises is invoked and replace the non-raw match strings with raw string equivalents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/harness/utils.py`:
- Around line 610-615: The tokens returned by shlex.split(...,
posix=(sys.platform != "win32")) on Windows keep surrounding quote characters,
so after calling shlex.split (the args variable) when running on win32 iterate
over args and strip a matching surrounding pair of single or double quotes from
each token (e.g., if token[0] == token[-1] and token[0] in ("'", '"') then
remove them) before applying the existing aiperf check; update the code that
constructs/returns args to perform this quote-stripping only when posix is False
(Windows) so paths like "C:\Users\..." become unquoted tokens.
In `@tests/unit/conftest.py`:
- Line 639: Add an explicit return type annotation to _patch_platform_constants:
annotate it as returning an ExitStack (i.e., def _patch_platform_constants(... )
-> ExitStack:) and ensure ExitStack is imported (or use contextlib.ExitStack or
typing.ContextManager[None] if you prefer). This matches its actual return (a
context manager) as used by the callers (_patch_platform_constants).
In `@tests/unit/zmq/test_dual_bind.py`:
- Around line 15-27: The _skip_on_windows_ipc pytest marker is defined in the
middle of the import block (between stdlib and project imports) causing E402;
move the definition of _skip_on_windows_ipc so that all imports (stdlib,
third-party, and aiperf/project imports like those currently below line 27)
appear first at the top of the module, then define _skip_on_windows_ipc
immediately after the import block to resolve the lint error; ensure the
identifier _skip_on_windows_ipc and its pytest.mark.skipif(...) expression are
unchanged apart from relocation.
---
Nitpick comments:
In `@tests/unit/zmq/test_dual_bind.py`:
- Line 119: The pytest.raises calls that use match="[Pp]ath is required" should
use a raw string to avoid regex escaping warnings: update the match argument in
the pytest.raises(ValueError, match=...) at the occurrence referencing
ValueError (line with match "[Pp]ath is required") and the other occurrence at
the later test (around the second occurrence) to use a raw string literal
(r"...") so the pattern is r"[Pp]ath is required"; locate these in
tests/unit/zmq/test_dual_bind.py where pytest.raises is invoked and replace the
non-raw match strings with raw string equivalents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9777c7bd-21d3-4e54-8b43-7ce58b6218f7
📒 Files selected for processing (22)
src/aiperf/cli_runner.pysrc/aiperf/common/bootstrap.pysrc/aiperf/common/config/zmq_config.pysrc/aiperf/common/constants.pysrc/aiperf/common/environment.pysrc/aiperf/common/tokenizer_display.pysrc/aiperf/controller/multiprocess_service_manager.pysrc/aiperf/controller/system_mixins.pysrc/aiperf/plugin/cli.pytests/harness/utils.pytests/unit/common/config/test_service_config.pytests/unit/common/config/test_zmq_config.pytests/unit/common/test_bootstrap_windows.pytests/unit/common/test_environment.pytests/unit/common/test_tokenizer_display.pytests/unit/conftest.pytests/unit/controller/test_system_mixins.pytests/unit/plugin/test_cli.pytests/unit/test_cli_runner_macos.pytests/unit/test_harness_utils.pytests/unit/transports/test_tcp_connector.pytests/unit/zmq/test_dual_bind.py
| # POSIX-mode shlex treats backslash as an escape character, which | ||
| # strips backslashes from Windows paths (C:\Users\... becomes | ||
| # C:Users...). On Windows we parse in non-POSIX mode so backslashes | ||
| # in interpolated paths are preserved. | ||
| args = shlex.split(cmd, posix=(sys.platform != "win32")) | ||
| return args[1:] if args and args[0] == "aiperf" else args |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import shlex
cmd = r'aiperf profile --file "C:\Users\test user\data.jsonl" --request-count 5'
print("posix=False:", shlex.split(cmd, posix=False))
print("posix=True :", shlex.split(cmd, posix=True))
PYRepository: ai-dynamo/aiperf
Length of output: 1955
🏁 Script executed:
python3 - <<'PY'
import shlex
# Test case: Windows path with spaces in quoted argument
cmd = r'aiperf profile --file "C:\Users\test user\data.jsonl" --request-count 5'
result_posix_false = shlex.split(cmd, posix=False)
result_posix_true = shlex.split(cmd, posix=True)
print("posix=False result:")
for i, arg in enumerate(result_posix_false):
print(f" [{i}] {repr(arg)}")
print("\nposix=True result:")
for i, arg in enumerate(result_posix_true):
print(f" [{i}] {repr(arg)}")
# Check if quotes are preserved
has_quotes_false = any('"' in arg for arg in result_posix_false)
print(f"\nQuotes preserved in posix=False: {has_quotes_false}")
print(f"Quotes preserved in posix=True: {any('"' in arg for arg in result_posix_true)}")
PYRepository: ai-dynamo/aiperf
Length of output: 295
🌐 Web query:
shlex.split posix parameter behavior quotes non-POSIX mode documentation
💡 Result:
In Python's shlex module, the posix parameter in shlex.split(s, comments=False, posix=True) determines whether the parser follows POSIX shell parsing rules [1][2]. When posix=True (the default), shlex adheres to standard shell parsing rules, including the handling of quotes and escape characters [1][3]. In this mode, quotes are stripped from the resulting tokens, and they do not act as word separators (e.g., "Do"Not"Separate" is parsed as the single token DoNotSeparate) [1][4]. When posix=False (non-POSIX mode), the parser uses a legacy compatibility mode [3][5]. This mode is generally simpler and does not handle backslash escaping or complex quoting rules as accurately as POSIX mode [3]. Specifically, in non-POSIX mode, quote characters are often preserved in the output tokens rather than being stripped, and backslashes may not be treated as escape characters [3][6]. In summary, if you are parsing command lines that resemble standard Unix-like shell syntax, using posix=True is recommended to ensure correct handling of quotes and escapes [3]. If you require the legacy behavior—where quotes might be preserved or parsing rules differ—you may use posix=False [3][6].
Citations:
- 1: https://docs.python.org/3/library/shlex.html
- 2: https://docs.python.org/3.8/library/shlex.html
- 3: https://runebook.dev/en/docs/python/library/shlex/shlex-objects
- 4: https://ironpython-test.readthedocs.io/en/latest/library/shlex.html
- 5: https://stackless.readthedocs.io/en/2.7-slp/library/shlex.html
- 6: https://stackoverflow.com/questions/20752751/split-multi-line-string-with-shlex-and-keep-quote-characters
🏁 Script executed:
python3 - <<'PY'
import shlex
# Test case: Windows path with spaces in quoted argument
cmd = r'aiperf profile --file "C:\Users\test user\data.jsonl" --request-count 5'
result_posix_false = shlex.split(cmd, posix=False)
result_posix_true = shlex.split(cmd, posix=True)
print("posix=False result:")
for i, arg in enumerate(result_posix_false):
print(f" [{i}] {repr(arg)}")
print("\nposix=True result:")
for i, arg in enumerate(result_posix_true):
print(f" [{i}] {repr(arg)}")
# Check if quotes are preserved
has_quotes_false = any('"' in arg for arg in result_posix_false)
has_quotes_true = any('"' in arg for arg in result_posix_true)
print(f"\nQuotes preserved in posix=False: {has_quotes_false}")
print(f"Quotes preserved in posix=True: {has_quotes_true}")
PYRepository: ai-dynamo/aiperf
Length of output: 421
Strip quotes from tokens when using non-POSIX shlex.split() on Windows.
When posix=False, shlex.split() preserves quote characters in the resulting tokens. On Windows, this means a quoted argument like "C:\Users\test user\data.jsonl" becomes a token with literal quotes: '"C:\\Users\\test user\\data.jsonl"'. Downstream code expecting unquoted paths will fail. Strip matching surrounding quote pairs on Windows to normalize behavior with the POSIX path:
Proposed fix
- args = shlex.split(cmd, posix=(sys.platform != "win32"))
+ if sys.platform == "win32":
+ args = shlex.split(cmd, posix=False)
+ # shlex non-POSIX keeps quote chars; normalize wrapped tokens.
+ args = [
+ arg[1:-1] if len(arg) >= 2 and arg[0] == arg[-1] == '"' else arg
+ for arg in args
+ ]
+ else:
+ args = shlex.split(cmd, posix=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/harness/utils.py` around lines 610 - 615, The tokens returned by
shlex.split(..., posix=(sys.platform != "win32")) on Windows keep surrounding
quote characters, so after calling shlex.split (the args variable) when running
on win32 iterate over args and strip a matching surrounding pair of single or
double quotes from each token (e.g., if token[0] == token[-1] and token[0] in
("'", '"') then remove them) before applying the existing aiperf check; update
the code that constructs/returns args to perform this quote-stripping only when
posix is False (Windows) so paths like "C:\Users\..." become unquoted tokens.
…ebased on v2 config) Signed-off-by: lkomali <lkomali@nvidia.com>
lkomali
force-pushed
the
lkomali/aiperf-windows-port
branch
from
4bb3ba9 to
bff6081
Compare
| if IS_WINDOWS: | ||
| salt = f"{path}/{ipc_filename}" | ||
| digest = hashlib.sha256(salt.encode()).hexdigest() | ||
| port_offset = int(digest[:8], 16) % _WINDOWS_TCP_PORT_RANGE |
There was a problem hiding this comment.
Hashing each Windows IPC endpoint into a fixed 20,000-port range can map two production sockets in the same run to the same TCP port, causing intermittent address already in use startup failures. Fix: allocate and share a collision-free per-run port map, or resolve collisions by probing/reserving ports before services start.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: lkomali <lkomali@nvidia.com>
4 participants
Summary
Foundational PR for native Windows support. Establishes the platform abstraction layer needed for
aiperf profileto run on Windows for low-concurrency single-shot benchmarks.What's in this PR
IS_WINDOWS/IS_MACOS/IS_LINUXcentralized incommon/constants.pyipc://); deterministic port fromsha256(path/ipc_filename)in range 28000–48000signal.signal()fallback for SIGINT + SIGBREAK on Windows (asyncioadd_signal_handleris POSIX-only)WindowsSelectorEventLoopPolicyswap before loop creation (pyzmq'sadd_reader/writeraren't on Proactor)ForkProcessdropped from type unions (Windows has noos.fork)[OK]instead of U+2713 in tokenizer logs (cp1252 safety)Stacked follow-up
lkomali/aiperf-windows-test-cleanupis the follow-up PR — adds Windows production fixes that surfaced through testing (SO_SNDBUF skip, SIGKILL→SIGTERM, path.anchor, DCGM auto-skip, dashboard non-TTY fallback) + the integration test infrastructure. Windows CI should remain disabled until that one lands too.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests