ci: fix unbound CONDA_PREFIX in test_doc_examples.sh - #1462
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ramakrishnap-nv
added
bug
|
/ok to test 60ef2ce |
|
/ok to test fca6276 |
fca6276 to
6a9a7d9
Compare
|
/ok to test 6a9a7d9 |
|
/ok to test 51de84a |
CONDA_PREFIX is unset in wheel test environments. Use \${CONDA_PREFIX:-}
so the variable defaults to empty rather than tripping set -o nounset.
The search loop already skips empty entries, so behavior is unchanged
in conda environments.
Fixes #1459.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The MIP C examples directory had .c files but no Makefile, causing test_doc_examples.sh to mark all 2 examples as failed. The convex examples Makefile is generic (auto-discovers .c files) so copy it here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
fd1f3e4 to
d0bf6f5
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates a shell script to handle an unset ChangescuOpt doc example updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@docs/cuopt/source/cuopt-c/mip/examples/Makefile`:
- Line 49: `LDFLAGS` in the MIP example Makefile assumes `LIBCUOPT_LIBRARY_PATH`
is a directory, but the documented setup can populate it with the full
`libcuopt.so` file path. Update the Makefile to normalize that variable before
using it with `-L` and `-Wl,-rpath`, or change the contract so
`LIBCUOPT_LIBRARY_PATH` is explicitly directory-only; use the `LDFLAGS`
assignment as the fix point and keep it consistent with the `mip-examples.rst`
setup instructions.
- Around line 58-69: The single-example build path bypasses the path validation
step, so `make <example_name>` can fail later with missing include/library flags
instead of a clear prerequisite message. Update the `Makefile` so `check-paths`
is also required by individual target builds, either by making `$(TARGETS)`
depend on it or by adding it to the `%: %.c` pattern rule alongside the existing
`all` target behavior. Keep the fix aligned with the `all` target and
`check-paths` logic so both full and single-example builds validate paths
consistently.
🪄 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: Enterprise
Run ID: afe6ab17-6bd6-4534-8204-e0aa7f618620
📒 Files selected for processing (2)
ci/test_doc_examples.shdocs/cuopt/source/cuopt-c/mip/examples/Makefile
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
It seems you might have included something you didn't mean to, will re-review once you clarify.
| @@ -0,0 +1,148 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
What's the goal with this file?
It seems totally unrelated to the title / description / linked issue.
There was a problem hiding this comment.
Apologies, so it started with doc testing failing silently and once I fixed it, there was another error where one of the test missed a Cmake file, so I fixed that, but missed to update the description.
There was a problem hiding this comment.
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
Ok got it, thanks. It's 2026, so let's please fix the copyright year here.
There was a problem hiding this comment.
Done — copyright updated to 2026.
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Approving so you have what you need to merge, do what you want with my small suggestions.
| # REQUIRED: Set these paths either: | ||
| # 1. As environment variables | ||
| # 2. On command line: make INCLUDE_PATH=... LIBCUOPT_LIBRARY_PATH=... | ||
| # 3. Uncomment and edit below for your system: | ||
| # | ||
| # INCLUDE_PATH = /path/to/cuopt/include | ||
| # LIBCUOPT_LIBRARY_PATH = /path/to/cuopt/lib |
There was a problem hiding this comment.
IMO it'd be simpler to just pick 1 way and document that that's how this works.
All this extra flexibility is just maintenance cost for no benefit unless you have a specific reason to want to support 3 different patterns.
The make INCLUDE_PATH=... form would be my preference.
There was a problem hiding this comment.
Done — simplified to command-line only (make INCLUDE_PATH=... LIBCUOPT_LIBRARY_PATH=...), removed env-var and hardcoded options. Also removed the verbose help/list targets.
- Update copyright year to 2026 - Simplify Makefile to single path-setting method (make VAR=...) - Add check-paths dependency to pattern rule so single-target builds also validate paths - Fix mip-examples.rst: use dirname so LIBCUOPT_LIBRARY_PATH is a directory as expected by -L Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/cuopt/source/cuopt-c/mip/mip-examples.rst`:
- Around line 25-26: The cuOpt examples are collecting every matching
libcuopt.so from find and passing the full list into dirname, which can produce
multiple library paths and break the later gcc -L usage. Update the libcuopt
path discovery in the relevant example blocks to select a single libcuopt.so
match first, then derive one directory from that single result, matching the
approach used in the doc-example flow. Use the existing LIBCUOPT_LIBRARY_PATH
setup and the later gcc invocation as the key spots to adjust so the examples
resolve one valid library path consistently.
🪄 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: Enterprise
Run ID: 20fe3b5c-be9c-4eec-81ea-47116bf3e36c
📒 Files selected for processing (2)
docs/cuopt/source/cuopt-c/mip/examples/Makefiledocs/cuopt/source/cuopt-c/mip/mip-examples.rst
| # Find the libcuopt library directory and assign to LIBCUOPT_LIBRARY_PATH | ||
| LIBCUOPT_LIBRARY_PATH=$(dirname $(find / -name "libcuopt.so" 2>/dev/null)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pick a single libcuopt.so match before deriving the directory.
Line 26 and Line 99 still feed every find result into dirname. On hosts with multiple cuOpt installs, that produces multiple directories, and the later gcc -L $LIBCUOPT_LIBRARY_PATH command splits them into separate arguments instead of one library path.
Proposed fix
- LIBCUOPT_LIBRARY_PATH=$(dirname $(find / -name "libcuopt.so" 2>/dev/null))
+ LIBCUOPT_LIBRARY_PATH=$(find / -name "libcuopt.so" 2>/dev/null | head -n 1)
+ LIBCUOPT_LIBRARY_PATH=${LIBCUOPT_LIBRARY_PATH%/libcuopt.so}As per path instructions, documentation changes should verify code examples compile and run correctly. Based on ci/test_doc_examples.sh:450-530, the doc-example flow also selects a single libcuopt.so hit before converting it to a directory.
Also applies to: 98-99
🤖 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 `@docs/cuopt/source/cuopt-c/mip/mip-examples.rst` around lines 25 - 26, The
cuOpt examples are collecting every matching libcuopt.so from find and passing
the full list into dirname, which can produce multiple library paths and break
the later gcc -L usage. Update the libcuopt path discovery in the relevant
example blocks to select a single libcuopt.so match first, then derive one
directory from that single result, matching the approach used in the doc-example
flow. Use the existing LIBCUOPT_LIBRARY_PATH setup and the later gcc invocation
as the key spots to adjust so the examples resolve one valid library path
consistently.
Source: Path instructions
CI Test Summary✅ All 31 test job(s) passed. |
|
/merge |
3 participants
${CONDA_PREFIX:-}so the variable defaults to empty when unset (wheel environments). The search loop skips empty entries, so conda behavior is unchanged. Fixes ci: test_doc_examples.sh fails silently in wheel-tests-cuopt-server due to unbound CONDA_PREFIX #1459.Makefiletodocs/cuopt/source/cuopt-c/mip/examples/; without it,test_doc_examples.shmarked all MIP C examples as failed.