Fix to decouple thin client requirements from cuopt main module #92
Conversation
rgsl888prabhu
added
bug
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
I don't think we should merge separate hard code status codes in the thin client. There should really only be one source of truth for these status values and that should be the C++ code.
|
@jameslamb May I get review on this PR |
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
This changes the entire Python API to use strings instead of status codes for termination status. I don't think that is what we want. We should just change the thin client to use strings. No changes need to be made in solution.py, solver_wrapper.pyx, or the tests of the Python API.
solution.py is packaged with thin client as well, so it would affect thin client if we keep it as it is. |
|
@chris-maes @rg20 Created new solution module for thin client and only server converts the termination status to string |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Approving from a ci-codeowners perspective, will defer to other reviewers on the substance of the code changes.
…bhu/cuopt_public into fix_thin_client_deps_on_cuopt
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
LGTM. One minor question. I see the addition of .name to the ProblemCategaory and the LPTermination status. Why is this needed?
|
It just changes enum value to it's corresponding string
It just changes enum value to it's corresponding string, instead of us manually doing it. |
4 participants
Termination status is being pulled from cuOpt which makes cuopt main library a dependency for cuopt thin client.
This PR converts termination status and problem category to string just for server and also adds a new solution class specific to thin client and detaches need to connect with enums from main cuOpt library.
closes #90