Conversation
--manifest is declared with resolve_path=True, so the CLI passes the runner a fully resolved path. The test asserted against the raw path returned by tempfile. On macOS the temp directory sits behind the /var -> /private/var symlink, so the two differ and test_run_passes_explicit_timeout_to_runner fails on a clean checkout. Compare against the resolved path instead. This is a no-op on platforms where the temp directory is already a real path.
The suite ran on ubuntu-latest only, so the macOS path failure fixed in the previous commit could not be caught upstream. Add one macos-latest job on a single interpreter rather than the full five-version matrix: macOS runners bill at a multiple of Linux, and the differences that bite here are filesystem behaviour rather than interpreter version. Set fail-fast: false so one platform failing does not cancel the others, which is the case this matrix exists to report.
README.md, CONTRIBUTING.md, and docs/quickstart.md each documented exactly one setup path, in PowerShell. CI runs Ubuntu only, so the documented path was never tested and the tested path was never documented. Add the macOS and Linux equivalents next to the existing Windows commands, and convert the PYTHONPATH examples in CONTRIBUTING.md to the POSIX form with a note on the PowerShell translation. The path separator differs there too, so the multi-path examples were wrong on macOS twice over. Also note the pip upgrade needed before the editable installs. Both packages build with hatchling, and the pip that ships with the system Python on macOS predates PEP 660 support, so 'pip install -e runtime/python' fails with 'Directory cannot be installed in editable mode'.
…HTTP server http.server.HTTPServer.server_bind calls socket.getfqdn() on the bound host to populate server_name. That is a reverse DNS lookup. Where the resolver is slow or has no PTR record for the address it blocks; measured at 35.01s for 127.0.0.1 on a GitHub macOS runner. TCPServer.__init__ binds the socket before that lookup and calls listen() only after it, so for the whole window the port is bound but refuses connections and serve_http has not yet printed its listening line. A caller sees connection refused rather than a server that is merely slow to start. This is what made the two HTTP example integration tests fail on macOS. Override server_bind to skip the lookup and use the host as given. server_name only ever reaches CGI-style headers. Also make the integration tests report why a server never came up: on timeout they now surface the server's exit code, stdout, and stderr instead of only the port number, and they fail immediately if the process has already exited.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The project does not build, test, or document cleanly on macOS. Adding macOS to CI turned up a real SDK bug behind the two HTTP integration tests, so this ended up larger than a docs patch.
1. A test fails on a clean checkout.
test_run_passes_explicit_timeout_to_runnerfails on macOS, 1 of 78:--manifestis declared withresolve_path=True(runtime/python/src/ecp_runtime/cli.py:62), so the CLI hands the runner a resolved path. The test asserted against the raw path fromtempfile, and on macOS the temp directory sits behind the/varto/private/varsymlink. Test bug, not a runtime bug.2.
serve_httpcan take 35 seconds to accept connections.http.server.HTTPServer.server_bindcallssocket.getfqdn()on the bound host to populateserver_name. That is a reverse DNS lookup, and where the resolver is slow or has no PTR record it blocks. Measured on a GitHub macOS runner:TCPServer.__init__binds the socket before that lookup and callslisten()only after it. For the whole 35 seconds the port is bound but refusing connections, andserve_httphas not yet printed its listening line, so a client sees connection refused rather than a server that is slow to start. This is what failstest_async_python_demo_conforms_over_httpandtest_streamable_http_demo_manifest_passeson macOS, and it would hit any user on a network with slow or absent reverse DNS, not only CI.3. CI could not catch either.
tests.ymlandci.ymlare bothruns-on: ubuntu-latestwith no OS axis.4. The docs are PowerShell-only.
README.md,CONTRIBUTING.md, anddocs/quickstart.mdeach give exactly one setup path, usingpy -m venvand.\.venv\Scripts\Activate.ps1. There are no macOS or Linux instructions anywhere. Since CI runs Ubuntu only, the documented path was never tested and the tested path was never documented. InCONTRIBUTING.mdthePYTHONPATHexamples are wrong on macOS twice over: the assignment syntax is PowerShell, and the multi-path examples use;where POSIX shells need:.Changes
sdk/python/src/ecp/server.py—_ECPThreadingHTTPServeroverridesserver_bindto skip thegetfqdnlookup and use the host as given.server_nameonly ever reaches CGI-style headers. Covered by a new test insdk/python/tests/test_server.pythat fails if the lookup returns.runtime/python/tests/test_cli.py— compare against the resolved path. A no-op where the temp directory is already real.runtime/python/tests/test_example_integrations.py— when a server never comes up, report the server's exit code, stdout, and stderr rather than only the port number, and fail immediately if the process has already exited. The original failure said onlyTimed out waiting for 127.0.0.1:49177, which is what made the cause above hard to see..github/workflows/tests.yml— add onemacos-latestjob, on a single interpreter rather than the full five-version matrix, since macOS runners bill at a multiple of Linux and the differences that bite here are filesystem and resolver behaviour rather than interpreter version. The matrix goes from 5 jobs to 6. Alsofail-fast: false, so one platform failing does not cancel the others.README.md,docs/quickstart.md,CONTRIBUTING.md— add macOS and Linux commands next to the existing Windows ones rather than replacing them, and convert theCONTRIBUTING.mdPYTHONPATHexamples to POSIX form with a note on the PowerShell translation.I did not use tabbed-content syntax, because
mkdocs.ymlenables onlyfenced_codeandtablesand it would render as literal text.One extra line in
CONTRIBUTING.md:python -m pip install --upgrade pipbefore the editable installs. Both packages build with hatchling, and the pip shipped with the system Python on macOS (21.2.4) predates PEP 660, so followingLocal Setupverbatim fails withDirectory cannot be installed in editable mode.Verification
Full matrix green on a fork run of this exact branch, including
macos-latest: 5 Ubuntu jobs (3.9 through 3.13) and the new macOS job.Locally on macOS 15 (arm64), from a clean virtualenv following the updated
CONTRIBUTING.mdverbatim:runtime/python/testssdk/python/testsruff check(asci.ymlruns it)mkdocs build --strictBoth test changes are load-bearing, confirmed by reverting each alone:
test_cli.pygoes 13 passed to 1 failure, and the new bind test fails when_build_http_serverreturns a plainThreadingHTTPServer.Notes
Two things found while verifying this that I did not touch, happy to file separately:
windows-latestto the matrix has a good argument, given the documented setup path is PowerShell and nothing tests it. Left out because I cannot verify it passes.ecp run --jsonwrites 164 bytes of LiteLLM banner to stdout ahead of the JSON when a judge grader errors withoutOPENAI_API_KEY, so the payloaddocs/ci.mdrecommends for CI is not parseable in that case. The JSON body is valid once the banner is stripped. Looks related to Move litellm to an optional extra #29.