Open
Conversation
rgsl888prabhu
added
doc
|
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)
📝 WalkthroughWalkthroughAdds an interactive install selector to cuOpt docs: new CSS/JS static assets, a Sphinx directive that injects the selector and writes a version JS file during build, and replaces in-page installation blocks across multiple quick-start guides with the selector plus a new install hub page. Changes
Estimated code review effort🎯 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![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 (3)
docs/cuopt/source/conf.py (1)
406-424: Remove redundantosimport inside function.The
osmodule is already imported at the top of the file (line 13), so the import on line 408 is unnecessary.♻️ Proposed fix
def write_install_version_js(app): """Write install selector version from cuopt.__version__ to output _static.""" - import os - outdir = getattr(app.builder, "outdir", None) or getattr( app.config, "outdir", None )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/conf.py` around lines 406 - 424, The function write_install_version_js contains a redundant local import "import os" (inside the function) even though os is already imported at file scope; remove that inner import so the function uses the module-level os import, keeping the rest of the function (outdir lookup, static_dir creation, file write using os.path.join) unchanged and run linting/formatting to ensure no unused-import warnings remain.docs/cuopt/source/cuopt-cli/quick-start.rst (1)
5-11: Consider adding a reference to the main installation page for consistency.Other quick-start pages (Python, C, Server) include "See :doc:
../installfor all interfaces and options." at the end of the installation guidance. Adding this here would maintain consistency across all quick-start guides.📝 Suggested addition
-cuopt_cli is built as part of the libcuopt package. Choose your install method below; the selector is pre-set for the CLI (it uses the same libcuopt install). Copy the command and run it in your environment. +cuopt_cli is built as part of the libcuopt package. Choose your install method below; the selector is pre-set for the CLI (it uses the same libcuopt install). Copy the command and run it in your environment. See :doc:`../install` for all interfaces and options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/cuopt-cli/quick-start.rst` around lines 5 - 11, Add the standard cross-reference line after the existing install selector block so this CLI quick-start matches the other guides: append the sentence "See :doc:`../install` for all interfaces and options." immediately after the ".. install-selector::" block (the block that has ":default-iface: cli") so users are directed to the main installation page.docs/cuopt/source/_static/install-selector.css (1)
116-126: Replace deprecatedclipproperty withclip-pathfor future compatibility.The
clipproperty on line 123 is deprecated. While this visually-hidden pattern works correctly, usingclip-pathis the modern approach.♻️ Proposed fix
.cuopt-opt input { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/_static/install-selector.css` around lines 116 - 126, The .cuopt-opt input rule uses the deprecated clip property; replace "clip: rect(0, 0, 0, 0);" with a modern clip-path declaration (e.g., clip-path: inset(0 0 0 0);) and include vendor fallback like -webkit-clip-path to maintain cross-browser behavior, keeping the other visual-hidden styles (position, width/height, overflow, white-space, border) intact to preserve accessibility and layout; update the selector .cuopt-opt input accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 279-288: The updateVisibility function currently shows the
"container" option even when COMMANDS[<iface>].container is null, allowing
selections that yield blank output; modify updateVisibility (and related
selection logic that reads getSelectedValue("cuopt-method") and
getSelectedValue("cuopt-iface")) to filter/hide method options based on the
chosen interface (e.g., only show methods where COMMANDS[iface][method] !==
null) and, if the currently selected method is not supported for that interface,
programmatically switch the method selection to the first supported method (or a
safe default) before calling updateOutput so no unsupported combination can be
selected and produce empty output. Ensure this validation uses the unique IDs
"cuopt-method" and "cuopt-iface" and checks COMMANDS[iface].container (and other
COMMANDS[iface][*]) to determine availability.
- Around line 251-253: The code currently remaps the selection variable iface
from "cli" to "c", causing the CLI choice to emit libcuopt (`c`) install
commands; remove that remapping (or change it to map "cli" to a distinct token
like "cli" or "cuopt_cli") so the CLI option is preserved and the downstream
command-generation logic (which reads iface) can emit correct cuopt_cli install
commands; update any downstream switch/lookup that expects "c" accordingly to
handle the preserved "cli"/"cuopt_cli" token.
- Around line 58-87: The conda install strings are incorrectly using the RAPIDS
package version variables V_CONDA/V_CONDA_NEXT for the cuda-version; update the
conda blocks for both stable and nightly so that the cu12 entries set
"cuda-version=12.9" and the cu13 entries set "cuda-version=13.0" (or 13.1 if you
intend that runtime), leaving the cuopt version pins as V_CONDA / V_CONDA_NEXT;
do this for the conda.stable.cu12, conda.stable.cu13, conda.nightly.cu12,
conda.nightly.cu13 entries (and the equivalent C-variant conda blocks) so the
commands use the correct CUDA runtime numbers instead of the package version
variables.
---
Nitpick comments:
In `@docs/cuopt/source/_static/install-selector.css`:
- Around line 116-126: The .cuopt-opt input rule uses the deprecated clip
property; replace "clip: rect(0, 0, 0, 0);" with a modern clip-path declaration
(e.g., clip-path: inset(0 0 0 0);) and include vendor fallback like
-webkit-clip-path to maintain cross-browser behavior, keeping the other
visual-hidden styles (position, width/height, overflow, white-space, border)
intact to preserve accessibility and layout; update the selector .cuopt-opt
input accordingly.
In `@docs/cuopt/source/conf.py`:
- Around line 406-424: The function write_install_version_js contains a
redundant local import "import os" (inside the function) even though os is
already imported at file scope; remove that inner import so the function uses
the module-level os import, keeping the rest of the function (outdir lookup,
static_dir creation, file write using os.path.join) unchanged and run
linting/formatting to ensure no unused-import warnings remain.
In `@docs/cuopt/source/cuopt-cli/quick-start.rst`:
- Around line 5-11: Add the standard cross-reference line after the existing
install selector block so this CLI quick-start matches the other guides: append
the sentence "See :doc:`../install` for all interfaces and options." immediately
after the ".. install-selector::" block (the block that has ":default-iface:
cli") so users are directed to the main installation page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cae7f371-a822-4f9e-9b2f-50a761a79dd7
📒 Files selected for processing (9)
docs/cuopt/source/_static/install-selector.cssdocs/cuopt/source/_static/install-selector.jsdocs/cuopt/source/conf.pydocs/cuopt/source/cuopt-c/quick-start.rstdocs/cuopt/source/cuopt-cli/quick-start.rstdocs/cuopt/source/cuopt-python/quick-start.rstdocs/cuopt/source/cuopt-server/quick-start.rstdocs/cuopt/source/index.rstdocs/cuopt/source/install.rst
Contributor
cwilkinson76
left a comment
cwilkinson76
left a comment
There was a problem hiding this comment.
Only a couple of minor edits needed, otherwise LGTM
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/cuopt/source/_static/install-selector.js (1)
27-33: Handle next-version rollover forYY.MMformat.
minor + 2can generate invalid month-like versions (e.g.,26.12→26.14). If the format isYY.MM, add year/month rollover logic.♻️ Proposed refactor
var parts = V_CONDA.split("."); var major = parseInt(parts[0], 10); var minor = parseInt(parts[1], 10) || 0; - var nextMinor = minor + 2; - var V_CONDA_NEXT = major + "." + (nextMinor < 10 ? "0" : "") + nextMinor; - var V_NEXT = major + "." + nextMinor; + var total = minor + 2; + var nextMajor = major + Math.floor((total - 1) / 12); + var nextMinor = ((total - 1) % 12) + 1; + var V_CONDA_NEXT = nextMajor + "." + (nextMinor < 10 ? "0" : "") + nextMinor; + var V_NEXT = nextMajor + "." + nextMinor;As per coding guidelines,
docs/**/*changes must prioritize Consistency: Version numbers, parameter types, and terminology match code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/_static/install-selector.js` around lines 27 - 33, The logic that computes V_CONDA_NEXT and V_NEXT by naively doing minor + 2 can produce invalid month-like versions for YY.MM formats; update the code around V_CONDA, V_CONDA_NEXT and V_NEXT to detect when V_CONDA uses a year.month convention (two numeric parts and minor between 0 and 12), then compute months = minor + 2, set yearIncrement = Math.floor(months / 12), nextMinor = months % 12 (treat 0 as 12 with appropriate year decrement if you prefer 1..12), nextMajor = major + yearIncrement, and format V_CONDA_NEXT and V_NEXT using nextMajor and a zero-padded nextMinor when needed; otherwise keep the existing simple increment behavior for non-month semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 237-257: The getCommand() logic reads hidden selectors (iface,
release, cuda) and can produce confusing output; update getCommand() to detect
when selectors are hidden or irrelevant: when iface === "cli" (normalized to
"c") do not read or depend on release/cuda values but use the appropriate
COMMANDS["c"] defaults; for method === "container" only use cuda if the selected
release entry actually contains cu12/cu13 keys (use fallback to a specific
available key instead of reading the UI selector); for non-container methods,
detect when data[release] is "default"-only and ignore the cuda selector (choose
data[release].default or the explicit cu12/cu13 keys that exist) so the emitted
cmd always matches visible UI choices; adjust the variables cudaKey/key and the
selection fallbacks to consult COMMANDS structure first rather than blindly
using the hidden selector values.
---
Nitpick comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 27-33: The logic that computes V_CONDA_NEXT and V_NEXT by naively
doing minor + 2 can produce invalid month-like versions for YY.MM formats;
update the code around V_CONDA, V_CONDA_NEXT and V_NEXT to detect when V_CONDA
uses a year.month convention (two numeric parts and minor between 0 and 12),
then compute months = minor + 2, set yearIncrement = Math.floor(months / 12),
nextMinor = months % 12 (treat 0 as 12 with appropriate year decrement if you
prefer 1..12), nextMajor = major + yearIncrement, and format V_CONDA_NEXT and
V_NEXT using nextMajor and a zero-padded nextMinor when needed; otherwise keep
the existing simple increment behavior for non-month semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bd48221-cd2c-409c-b25d-69e6a82dd867
📒 Files selected for processing (5)
docs/cuopt/source/_static/install-selector.cssdocs/cuopt/source/_static/install-selector.jsdocs/cuopt/source/conf.pydocs/cuopt/source/cuopt-cli/quick-start.rstdocs/cuopt/source/install.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/cuopt/source/cuopt-cli/quick-start.rst
- docs/cuopt/source/install.rst
…_public into add_install_selector
Member
|
Maybe we could have this in the repo README as well? |
Iroy30
reviewed
Mar 17, 2026
Collaborator
Author
Repo README.md can't directly host the UI, but we can link this to docs, but the issue is the current doc doesn't have this, so it will break the details. |
Iroy30
approved these changes
Mar 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/cuopt/source/_static/install-selector.js (2)
311-325: Consider using modern Clipboard API.
document.execCommand("copy")is deprecated. The modernnavigator.clipboard.writeText()API provides better async handling and broader future compatibility.♻️ Proposed refactor using Clipboard API with fallback
function copyToClipboard() { var out = document.getElementById("cuopt-cmd-out"); if (!out.value) return; - out.select(); - out.setSelectionRange(0, 99999); - try { - document.execCommand("copy"); - var btn = document.getElementById("cuopt-copy-btn"); - var orig = btn.textContent; - btn.textContent = "Copied!"; - setTimeout(function () { - btn.textContent = orig; - }, 1500); - } catch (e) {} + var btn = document.getElementById("cuopt-copy-btn"); + var orig = btn.textContent; + function showFeedback() { + btn.textContent = "Copied!"; + setTimeout(function () { btn.textContent = orig; }, 1500); + } + if (navigator.clipboard && navigator.clipboard.writeText) { + navigator.clipboard.writeText(out.value).then(showFeedback).catch(function () {}); + } else { + out.select(); + out.setSelectionRange(0, 99999); + try { document.execCommand("copy"); showFeedback(); } catch (e) {} + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/_static/install-selector.js` around lines 311 - 325, Update the copyToClipboard function to use the modern asynchronous Clipboard API: replace the synchronous document.execCommand("copy") block with an await navigator.clipboard.writeText(out.value) call (inside an async function) and preserve UX behavior by updating the button text via document.getElementById("cuopt-copy-btn") on success; include a try/catch that falls back to the existing execCommand approach when navigator.clipboard is unavailable or the writeText call fails, so the function still handles older browsers and retains the "Copied!" temporary text restoration logic.
368-377: Default interface applied after initial render causes redundant update.The
data-default-ifaceattribute is processed after the firstupdateVisibility()call at Line 368, triggering a second update at Line 375. This could cause a brief visual flicker.♻️ Proposed fix to check default before first updateVisibility
document.getElementById("cuopt-copy-btn").addEventListener("click", copyToClipboard); - updateVisibility(); var defaultIface = root.getAttribute("data-default-iface"); if (defaultIface && ["python", "c", "server", "cli"].indexOf(defaultIface) !== -1) { var radio = document.querySelector('input[name="cuopt-iface"][value="' + defaultIface + '"]'); if (radio) { radio.checked = true; - updateVisibility(); } } + updateVisibility(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/_static/install-selector.js` around lines 368 - 377, The initial call to updateVisibility runs before honoring the data-default-iface attribute causing a redundant second update and possible flicker; change the initialization order so you read root.getAttribute("data-default-iface") and, if defaultIface is valid and the matching input (querySelector for 'input[name="cuopt-iface"][value="' + defaultIface + '"]') exists, set radio.checked = true first, then call updateVisibility() only once; locate the code that currently calls updateVisibility() prior to the defaultIface handling and move or branch that call to occur after the defaultIface/radio.checked logic (references: updateVisibility, defaultIface, root, radio).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 157-169: The cu13 pip string in the stable selector is using the
wrong CUDA runtime package name; update the cu13 entry under the stable object
(the "cu13" key in install-selector.js) to use
"nvidia-cuda-runtime-cu13==13.0.*" (keeping the surrounding concatenation with V
and V_CONDA unchanged) so it mirrors the cu12 pattern and matches V and V_CONDA
usage.
---
Nitpick comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 311-325: Update the copyToClipboard function to use the modern
asynchronous Clipboard API: replace the synchronous document.execCommand("copy")
block with an await navigator.clipboard.writeText(out.value) call (inside an
async function) and preserve UX behavior by updating the button text via
document.getElementById("cuopt-copy-btn") on success; include a try/catch that
falls back to the existing execCommand approach when navigator.clipboard is
unavailable or the writeText call fails, so the function still handles older
browsers and retains the "Copied!" temporary text restoration logic.
- Around line 368-377: The initial call to updateVisibility runs before honoring
the data-default-iface attribute causing a redundant second update and possible
flicker; change the initialization order so you read
root.getAttribute("data-default-iface") and, if defaultIface is valid and the
matching input (querySelector for 'input[name="cuopt-iface"][value="' +
defaultIface + '"]') exists, set radio.checked = true first, then call
updateVisibility() only once; locate the code that currently calls
updateVisibility() prior to the defaultIface handling and move or branch that
call to occur after the defaultIface/radio.checked logic (references:
updateVisibility, defaultIface, root, radio).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab1a90ab-1da6-4080-be3d-5658d9d0797a
📒 Files selected for processing (1)
docs/cuopt/source/_static/install-selector.js
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
4 participants
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.
Description
This PR adds support for install selector instead of static instructions.
Issue
closes #954
Checklist