[Mohammed Alkindi (MohammedAlkindi)]

The documented dev setup leaves a contributor without the pinned onnx-weekly.

requirements-dev.txt:3 pins onnx-weekly>=1.17.0.dev20240325. The next line, pip install -e ., resolves onnx>=1.17 from pyproject.toml, and pip treats onnx and onnx-weekly as unrelated distributions, so it installs stable onnx on top of the weekly build.

Measured in a clean venv on Python 3.13:

pip install "onnx-weekly>=1.17.0.dev20240325"   ->  onnx-weekly 1.24.0.dev20260914
pip install --dry-run "onnx>=1.17"             ->  Collecting onnx>=1.17
                                                   (numpy, protobuf, typing_extensions,
                                                    ml_dtypes already satisfied)

pip reports the transitive dependencies as already satisfied but still collects onnx itself, so the editable install replaces what the previous line pinned.

noxfile.py already passes --no-deps at every install of the package, at lines 67, 83, 84, 94, 96 and 115, which is the same guard applied to CI but not to the documented path.

Running python docs/update_readme.py after the change produces no further diff, so the update_readme job's git diff --exit-code -- README.md still passes.

[Copilot]

Copilot review overview

🟢 Approval recommended

The documentation change correctly aligns local setup with existing nox installation behavior.

Review effort: Balanced
Findings: None

What changed in this PR

Updates development installation instructions to preserve the pinned onnx-weekly package.

Changes:

• Adds --no-deps to the editable package installation.

File	Description
README.md	Prevents pip from installing stable onnx over onnx-weekly.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.01%. Comparing base (08898ad) to head (6e537b7).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3051      +/-   ##
==========================================
+ Coverage   72.73%   73.01%   +0.27%     
==========================================
  Files         265      267       +2     
  Lines       32358    32641     +283     
  Branches     3069     3101      +32     
==========================================
+ Hits        23536    23832     +296     
+ Misses       7779     7767      -12     
+ Partials     1043     1042       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[G. Ramalingam (gramalingam)]

Blocking finding

High: the documented clean development setup no longer installs onnx_ir

Location: README.md:63, changed command pip install -e . --no-deps.

The preceding pip install -r requirements-dev.txt does not install the mandatory onnx_ir dependency: that requirements file contains neither onnx_ir nor onnxscript. The package's pyproject.toml declares onnx_ir>=0.1.16,<2, and onnxscript/_internal/main.py:11 imports it unconditionally. Before this change, the editable install supplied that dependency. Adding --no-deps removes that step, so a clean environment following these instructions can fail immediately on import onnxscript with ModuleNotFoundError: No module named 'onnx_ir'.

The motivation to avoid installing stable ONNX over onnx-weekly is valid, but disabling all dependency resolution requires the prerequisite requirements to supply all runtime dependencies. The cited nox precedent is not equivalent: nox explicitly includes/installs ONNX_IR before its no-dependencies package installation.

Recommended fix: update the development requirements or the documented prerequisite command to explicitly install the supported onnx_ir version, and audit the other package runtime requirements for completeness. Keep --no-deps once those prerequisites are supplied.

Confidence: high. Evidence is from the exact-head dependency manifest, development requirements, unconditional import, and nox installation sequence. This is a newly introduced dependency omission, not an objection to the existing ONNX-weekly setup.

[G. Ramalingam (gramalingam)]

Copilot indicates potential issues

[Mohammed Alkindi (MohammedAlkindi)]

Done in 6e537b7. The README now installs onnx_ir>=0.1.16,<2 with --no-deps before the editable install, as nox does; onnx_ir pulls stable onnx, so adding it to requirements-dev.txt would displace onnx-weekly. The other runtime deps already resolve from requirements-dev.txt. In a clean venv with onnx-weekly 1.24.0.dev20261005, import onnxscript works after the new step and fails with No module named 'onnx_ir' without it.

[Copilot]

🟢 Approval recommended

The documented installation sequence correctly avoids conflicting dependency resolution.

0 open findings

🧠 Review effort: Balanced