Skip to content

Skip version check for cuOpt in Julia code#539

Merged
rgsl888prabhu merged 10 commits intoNVIDIA:mainfrom
rgsl888prabhu:fix_jump_testing
Oct 29, 2025
Merged

Skip version check for cuOpt in Julia code#539
rgsl888prabhu merged 10 commits intoNVIDIA:mainfrom
rgsl888prabhu:fix_jump_testing

Conversation

@rgsl888prabhu
Copy link
Collaborator

@rgsl888prabhu rgsl888prabhu commented Oct 24, 2025

Description

cuOpt.jl checks for cuOpt version to be <= latest release. But for CI testing we need that check to be disabled, so this change will update the max version to 99.99 so we can get pass that.

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

Summary by CodeRabbit

  • Tests
    • Jump tests now run unconditionally in more workflows in addition to nightly runs, increasing test coverage.
    • Improved third‑party optimizer test reliability: relaxed strict version checks, skipped a flaky test, enhanced native library discovery, and added clearer logging to aid diagnostics across environments.

@rgsl888prabhu rgsl888prabhu self-assigned this Oct 24, 2025
@rgsl888prabhu rgsl888prabhu requested a review from a team as a code owner October 24, 2025 18:52
@rgsl888prabhu rgsl888prabhu added the non-breaking Introduces a non-breaking change label Oct 24, 2025
@rgsl888prabhu rgsl888prabhu requested review from gforsyth and removed request for a team October 24, 2025 18:52
@rgsl888prabhu rgsl888prabhu added the improvement Improves an existing functionality label Oct 24, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

Walkthrough

Updated a comment in ci/test_wheel_cuopt.sh describing the nightly-tests section; ci/thirdparty-testing/run_jump_tests.sh now (1) mutates src/cuOpt.jl max-version to v"99.99" when present, (2) comments out test_air05 in test/MOI_wrapper.jl, (3) includes guarded libcuopt.so discovery and LD_LIBRARY_PATH logging, and (4) shortens a Julia invocation log line.

Changes

Cohort / File(s) Summary
CI comment update
ci/test_wheel_cuopt.sh
Revised the preceding comment describing the nightly-tests section to refer to “thirdparty integration tests” instead of “jump tests and cvxpy integration tests.” No functional changes.
Run-jump-tests script — version patch
ci/thirdparty-testing/run_jump_tests.sh
If src/cuOpt.jl exists, mutate its max-version pattern to v"99.99" (via sed) with surrounding logging.
Run-jump-tests script — test patch
ci/thirdparty-testing/run_jump_tests.sh
If test/MOI_wrapper.jl exists, comment out the test_air05 function (skip that test) and log the action.
Run-jump-tests script — library discovery & logging
ci/thirdparty-testing/run_jump_tests.sh
Add a guarded block to locate libcuopt.so; if found, prepend its directory to LD_LIBRARY_PATH and emit detailed logs; if not found, emit a warning.
Run-jump-tests script — logging tweak
ci/thirdparty-testing/run_jump_tests.sh
Shorten the Julia test-invocation message to a less verbose line (removes the “may spew output loudly” phrasing).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Potential review focus:

  • Verify the sed pattern used to change the max-version in src/cuOpt.jl is safe and only matches intended lines.
  • Ensure commenting out test_air05 in test/MOI_wrapper.jl doesn't mask other required behavior.
  • Confirm libcuopt.so discovery and LD_LIBRARY_PATH handling work across CI environments and do not introduce path side effects.
  • Quick sanity-check of log message changes for clarity.

Poem

🐰 I tweak a version, soft and light,

I hush one test that wakes at night,
I sniff for libs and mark the trail,
A shorter note, a tidy tail,
Hoppity hop — the CI's bright.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Skip version check for cuOpt in Julia code" directly and accurately reflects the primary objective of this PR, which is to bypass the cuOpt.jl version constraint check by setting the maximum version to v"99.99" in the CI testing context. The title is concise, clear, and specific enough that a reviewer can immediately understand the main purpose without ambiguity. While the PR includes supporting changes such as patching MOI_wrapper.jl and adding LD_LIBRARY_PATH handling, the title appropriately focuses on the core objective as described in the PR description. The title is not vague, generic, or misleading, and it clearly summarizes the most important change from the developer's perspective.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a63e1be and 5824c51.

📒 Files selected for processing (2)
  • ci/test_wheel_cuopt.sh (1 hunks)
  • ci/thirdparty-testing/run_jump_tests.sh (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • ci/test_wheel_cuopt.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • ci/thirdparty-testing/run_jump_tests.sh

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ci/test_wheel_cuopt.sh (1)

79-85: Remove the duplicate cvxpy test execution within the nightly conditional block.

The git history confirms this is a bug. Commit a57d894 moved cvxpy tests into the nightly-only conditional, but commit 2e9625a (today) re-added the unconditional cvxpy execution at line 85 without removing the duplicate at line 82. This causes cvxpy tests to run twice on nightly builds.

The original intent was to run cvxpy on all builds and jump tests only on nightly builds. Remove line 82 to restore that behavior:

if [[ "${RAPIDS_BUILD_TYPE}" == "nightly" ]]; then
    ./ci/thirdparty-testing/run_jump_tests.sh
fi

./ci/thirdparty-testing/run_cvxpy_tests.sh
🧹 Nitpick comments (1)
ci/thirdparty-testing/run_jump_tests.sh (1)

43-49: Improve sed pattern robustness and add validation.

The sed regex uses greedy matching (min.*max.*=.*v") which could unintentionally match across multiple lines or statements if the cuOpt.jl source has multiple version constraints. Additionally, there's no validation that the pattern was actually found and replaced.

Suggested improvements:

  1. More specific pattern: Anchor to line boundaries or use more precise matching to avoid greedy overreach.
  2. Validation: Check if the replacement succeeded, and log or fail if the expected pattern is not found.

For example:

if sed -i 's/\(min.*max.*=.*v"[0-9]\+\.[0-9]\+"\),\s*v"[0-9]\+\.[0-9]\+"/\1, v"99.99"/g' src/cuOpt.jl; then
    if grep -q 'v"99.99"' src/cuOpt.jl; then
        rapids-logger "Patch applied successfully; verified v\"99.99\" in src/cuOpt.jl"
    else
        rapids-logger "Warning: sed completed but version string not found in result"
    fi
fi

Alternatively, if you have the exact line format from cuOpt.jl, consider using a more specific pattern that matches only the intended context.

Please verify the sed pattern works correctly with the actual cuOpt.jl source file. Consider running the script in a test environment and inspecting src/cuOpt.jl to confirm the replacement is correct and no unintended changes occur.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a814d5 and 2e9625a.

📒 Files selected for processing (2)
  • ci/test_wheel_cuopt.sh (1 hunks)
  • ci/thirdparty-testing/run_jump_tests.sh (1 hunks)

./ci/thirdparty-testing/run_cvxpy_tests.sh
fi

./ci/thirdparty-testing/run_jump_tests.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, just left it out to test, once it completes the run, I will move it back.

@rgsl888prabhu
Copy link
Collaborator Author

@gforsyth May I get your review on this PR

@rgsl888prabhu rgsl888prabhu merged commit b90543a into NVIDIA:main Oct 29, 2025
172 of 175 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants