-
Notifications
You must be signed in to change notification settings - Fork 47
add a simple smoketest for cvxpy integration #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: branch-25.08
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments for your consideration.
@jameslamb Thank you for the review, I had addressed few of the review comments and will follow-up on any new ones. |
@@ -59,4 +59,29 @@ timeout 10m bash ./python/libcuopt/libcuopt/tests/test_cli.sh | |||
# Run Python tests | |||
RAPIDS_DATASET_ROOT_DIR=./datasets timeout 30m python -m pytest --verbose --capture=no ./python/cuopt/cuopt/tests/ | |||
|
|||
timeout 3m ./ci/external/cvxpy_smoketest.sh | |||
# run cvxpy integration tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jameslamb We would have more integration tests in future, should we move few items to be partly into a bash script and run it from there to keep the test_wheel_cuopt.sh simple ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure we can, and to be fair that's how @tmckayus had it originally before I pushed commits here.
Let me tell you why I did it this way. I was mainly concerned about making sure that the pip install
that installs cvxpy
uses the just-built-in-this-CI-run cuopt
packages.
Doing it in 1 script like this makes that easy to guarantee, because I can just reference variables from the same scope and packages downloaded a few lines higher up. Like this:
CUOPT_SH_CLIENT_WHEELHOUSE=$(RAPIDS_PY_WHEEL_NAME="cuopt_sh_client" rapids-download-wheels-from-github python)
python -m pip install \
--extra-index-url=https://pypi.nvidia.com \
"${CUOPT_MPS_PARSER_WHEELHOUSE}"/cuopt_mps_parser*.whl \
"${CUOPT_SH_CLIENT_WHEELHOUSE}"/cuopt_sh_client*.whl \
"${CUOPT_WHEELHOUSE}"/cuopt*.whl \
"${LIBCUOPT_WHEELHOUSE}"/libcuopt*.whl \
'pytest-error-for-skips>=2.0.2' \
"$(echo ./dist/cvxpy*.whl)[CUOPT,testing]"
If you want to defer all the cvxpy
stuff to a separate script, like Trevor had it before, it'd involve some changes to how packages are downloaded.
I can do that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we might be adding few more smoke tests to this, I felt the script might become too huge and it might not be scaled. Yeah, lets move these to a folder "thirdparty-testing" and add a script there to trigger test. Please let me know if your hands are full, I can work on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will push changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed changes moving cvxpy
tests to their own file, as @tmckayus had originally (but using the name thirdparty-testing/
you asked for).
Hopefully this will set up a pattern that can be reliably replicated when other integration tests like this are added.
I will check back in an hour or so to be sure this worked. I especially want to see evidence in the logs that the just-built-in-CI cuopt
, cuopt-mps-parser
, etc. wheels were used, not packages downloaded from the nightly index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jameslamb
I've removed my blocking review. Sorry @rgsl888prabhu , I wasn't able to get this done today, and I'll be unavailable all of next week. Hopefully I've written enough here to explain my concerns with the original approach and show the direction I want to go. Talk with others in |
This is a simple smoketest to look for breakage and any abnormal behavior with respect to runtime from cuopt Python API changes. It does not do any exhaustive performance testing or building.