Skip to content

CuOpt Solver Integration #839

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

Merged
merged 11 commits into from
May 29, 2025
Merged

CuOpt Solver Integration #839

merged 11 commits into from
May 29, 2025

Conversation

Iroy30
Copy link
Contributor

@Iroy30 Iroy30 commented May 19, 2025

No description provided.

@CLAassistant
Copy link

CLAassistant commented May 19, 2025

CLA assistant check
All committers have signed the CLA.

@Iroy30 Iroy30 mentioned this pull request May 19, 2025
@Iroy30
Copy link
Contributor Author

Iroy30 commented May 19, 2025

Incorporated review comments from #837

@pchtsp
Copy link
Collaborator

pchtsp commented May 19, 2025

Thanks. I'm not sure why you closed the previous PR. As long as you push the code to the branch, you can leave the PR open and it will update with the new changes. This makes easier to follow the changes/ suggestions and comments.

For example, there seems to be an error with the types when checking them with mypy, as I mentioned in the previous PR. I've added instructions to run mypy locally in the contributing site: https://coin-or.github.io/pulp/develop/contribute.html#checking-types-with-mypy

Also, you seem to not have signed the CLA in this PR.

@bbozkaya
Copy link

Hi, we have re-submitted this PR via the NVidia fork of the Pulp repo using NV org account, as discussed with COIN-OR and agreed upon. All the reviews requested for the original PR are addressed.
#839

Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

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

as I mentioned, running mypy ./ is still failing. I've added the lines that need to be changed in the code.

warmStart=warmStart,
)

from cuopt.linear_programming import data_model
Copy link
Collaborator

Choose a reason for hiding this comment

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

change for:

            from cuopt.linear_programming import data_model  # type: ignore[import-not-found, import-untyped, unused-ignore]


try:
global cuopt
import cuopt # type: ignore[import-not-found]
Copy link
Collaborator

Choose a reason for hiding this comment

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

change for:

import cuopt  # type: ignore[import-not-found, import-untyped, unused-ignore]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

@pchtsp
Copy link
Collaborator

pchtsp commented May 20, 2025

I'd do the changes myself but I cannot push to nvidia/pulp/master it seems. You also need to merge the current master into the branch

@Iroy30
Copy link
Contributor Author

Iroy30 commented May 22, 2025

@pchtsp Currently I am running all the pulp tests in test_pulp.py with CUOPT as the solver instance (Updated the test in the PR). I can paste the results as a comment here since the CI cannot run cuopt. Is there anything else we need to verify wrt testing?

@Iroy30
Copy link
Contributor Author

Iroy30 commented May 27, 2025

Also mypy has started failing again. I can fix the type issue in test_pulp.py , however, I cannot reproduce the numpy import issue in apis/cuopt_api.py
Could you confirm it would be the right place to do a numpy import ?

@pchtsp
Copy link
Collaborator

pchtsp commented May 28, 2025

hello, thanks for the reminder to look into this. Two things:

  1. I fixed the typing issue in the master branch, you should just need to pull the master again.
  2. for numpy I'd recommend just doing the same as for the other libraries: import numpy as np # type: ignore[import-not-found, import-untyped, unused-ignore] and hope it doesn't complain.

thanks and let's hope this is the last change : )

@Iroy30
Copy link
Contributor Author

Iroy30 commented May 28, 2025

@pchtsp The final question :)
#839 (comment)

@pchtsp
Copy link
Collaborator

pchtsp commented May 29, 2025

Sorry I forgot to answer, yes: please paste here all the logs from the tests so we can take a look. Ideally for the next release, we should try to set up the github actions. But with additional solvers what we mostly care is that we're not breaking other solvers, which doesn't seem the case. I will merge as soon as all the tests pass

@pchtsp pchtsp merged commit 0d6e0a5 into coin-or:master May 29, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants