Skip to content
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

Support for upcoming knitro python package #3478

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tvignon-artelys
Copy link

Fixes # .

Summary/Motivation:

We at Artelys are currently developing a Python package for Knitro which will be released soon. This package will contain among other things a knitroampl executable, which is the one called when solving a Pyomo model with Knitro. We prefer our package not to modify environment variables such as the PATH like the packages for Xpress or other solvers do. This pull request provides simple and seamless integration between Pyomo and the upcoming Knitro python package.

Changes proposed in this PR:

  • When calling solver 'knitroampl', check if there is a knitro package installed in the same python environment as pyomo, and if it contains the knitroampl executable. If this condition is met, use that executable instead of searching in the PATH
  • If one of the above conditions is not met, no changes in behavior

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@tvignon-artelys tvignon-artelys changed the title Feature/support for knitro python package Support for upcoming knitro python package Feb 19, 2025
@mrmundt
Copy link
Contributor

mrmundt commented Feb 19, 2025

Hi @tvignon-artelys - immediate feedback is to please add tests to ensure that the interface is working as intended.

@tvignon-artelys
Copy link
Author

Hi @tvignon-artelys - immediate feedback is to please add tests to ensure that the interface is working as intended.

Hi @mrmundt , I don't quite see what kind of tests I could add.
There are already a few tests checking that the knitroampl interface works, but from what I can see they never run in the pipeline, presumably because the pipeline environment doesn't seem to install Knitro.
More importantly, in order to check that the functionality added by this PR works properly, we would need to first install the Knitro solver from source, run the tests, then uninstall it, install the knitro package from PyPI, and run the same tests again. Do these 5 lines of new code warrant such a complicated testing workflow ?
The only thing this PR does is check if an executable exists and if yes set it as the executable to be used.

Best regards,

@tvignon-artelys
Copy link
Author

Also the Read the Docs build appears to have failed for reasons that are unclear to me and can not be related to the modification in this PR

@mrmundt
Copy link
Contributor

mrmundt commented Feb 19, 2025

Also the Read the Docs build appears to have failed for reasons that are unclear to me and can not be related to the modification in this PR

Re this: Confirmed, not related to you. I have a ticket open with sphinx about it.

I have opened #3479 as a stop-gap until the true issue is fixed.

@mrmundt
Copy link
Contributor

mrmundt commented Feb 19, 2025

Hi @tvignon-artelys - immediate feedback is to please add tests to ensure that the interface is working as intended.

Hi @mrmundt , I don't quite see what kind of tests I could add. There are already a few tests checking that the knitroampl interface works, but from what I can see they never run in the pipeline, presumably because the pipeline environment doesn't seem to install Knitro. More importantly, in order to check that the functionality added by this PR works properly, we would need to first install the Knitro solver from source, run the tests, then uninstall it, install the knitro package from PyPI, and run the same tests again. Do these 5 lines of new code warrant such a complicated testing workflow ? The only thing this PR does is check if an executable exists and if yes set it as the executable to be used.

Best regards,

We would not require multiple methods of installation; one would be sufficient to ensure functionality. We recommend adding the logic for installation to the two test_* workflows: https://github.com/Pyomo/pyomo/tree/main/.github/workflows

You can view other PRs that added support for different solvers for examples of what testing looked like, e.g., #3200 - https://github.com/Pyomo/pyomo/pull/3200/files#diff-4b8b6f8dad3b568afc0c32a308cf635fc6469a5196aba0ecacf850697d226c7f, https://github.com/Pyomo/pyomo/pull/3200/files#diff-1187672cba3850c20aff8ae1b0e9e978d0431611be69458787407903f6cda984

@tvignon-artelys
Copy link
Author

Hi,
Knitro does not have a demo mode which runs without a license, which makes this type of test complicated for the same reason that you have GAMS version pinned to 29.1.0. I looked at the two workflows but could not find an example of solver installation which requires a license.

@jsiirola
Copy link
Member

Pyomo has two testing frameworks. The tests on GHA test everything that can run without licenses (we have not identified a suitable approach to managing licenses on a public testing infrastructure).

Testing that requires a license (either because the test is too large for a solver's demo mode or because the solver lacks a demo mode) are run by a Jenkins instance using machines that are internal to Sandia and host software licenses that have been granted by the solver vendors for the purpose of testing Pyomo.

@tvignon-artelys
Copy link
Author

@jsiirola Thanks for your reply ! Given that tests for Knitro already exist in the workflows, I imagine that they are run on the internal Sandia Jenkins instance. What extra testing would you then expect for this PR ?

@jsiirola
Copy link
Member

@tvignon-artelys: we do not have a license for knitro - those tests are skipped on all our testing platforms. I believe those tests were stubbed in more than 10 years ago by an external partner who was using knitro, but have never been run by the automated CI/CD systems.

Right now, there is no knitro-specific code in the code base: the solver runs through the generic ASL interface. As we cannot currently test knitro, I don't think we call it an "officially" supported solver. This PR would change that, and to @mrmundt's point, if we have solver-specific code in the project, we should find a way to test it / verify it actually works (both now and into the future).

Separately, I haven't had a chance to put together a review of this PR, but I think we will want to take a different approach. Solver-specific code should really be in solver-specific derived classes and not in the base class (this is what we do for Ipopt and Conopt). In addition, it would probably be better to work directly with the underlying pyomo.common.Executable() infrastructure instead of rewriting the arguments to the generic interface.

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.

3 participants