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

Add SCIP solver for testing #1282

Merged
merged 7 commits into from
Nov 17, 2023
Merged

Conversation

lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl lbianchi-lbl commented Nov 1, 2023

Resolves #1281

Changes proposed in this PR:

  • Add non-public util function to enable SCIP solver (using ampl_module_scip)
  • Add scip_solver pytest fixture for model diagnostics tests
    • If a test uses the scip_solver fixture (i.e. requires SCIP to run), it will be automatically skipped without the need of adding individual pytest.mark.skipif() markers if SCIP is not available
  • Add step in GHA installation action to install ampl_module_scip from https://pypi.ampl.com

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  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.

@lbianchi-lbl lbianchi-lbl self-assigned this Nov 1, 2023
@lbianchi-lbl lbianchi-lbl added the Priority:High High Priority Issue or PR label Nov 1, 2023
@lbianchi-lbl lbianchi-lbl marked this pull request as ready for review November 1, 2023 18:55

def undo_changes():
try:
os.environ["PATH"].remove(new_path_entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ughhh, I just realized that this won't work since os.environ["PATH"] is a str rather than a list. Serves me right for trying to be smart and implement a way to revert global changes to the environment...

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d2f24ad) 77.33% compared to head (986ba84) 77.40%.

Files Patch % Lines
idaes/core/util/testing.py 76.19% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1282      +/-   ##
==========================================
+ Coverage   77.33%   77.40%   +0.07%     
==========================================
  Files         390      390              
  Lines       63367    63388      +21     
  Branches    11648    11649       +1     
==========================================
+ Hits        49006    49068      +62     
+ Misses      11839    11796      -43     
- Partials     2522     2524       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


try:
path_as_list.remove(new_path_entry)
except ValueError:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a bad idea to add a test to cover this case to future-proof against any changes in how AMPL and/or Python handle their PATH entries / modifications in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it'd be nice to have the "undo" functionality tested. It's a bit tricky but I'll see what I can do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrmundt I've added a basic test for the PATH manipulation: 986ba84

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @lbianchi-lbl ! Can I note how funny this name is, though? test for testing ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noting, that was very much on purpose! I like to think of it as "Barber paradox for software engineers" 😅

@lbianchi-lbl lbianchi-lbl added the CI:run-integration triggers_workflow: Integration label Nov 16, 2023
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Nov 16, 2023
@lbianchi-lbl lbianchi-lbl merged commit ccf34ba into IDAES:main Nov 17, 2023
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling SCIP as an available solver for testing
4 participants