Skip to content

Use shutil.which() to locate gams #564

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 29, 2025
Merged

Use shutil.which() to locate gams #564

merged 3 commits into from
Apr 29, 2025

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Mar 25, 2025 •
edited
Loading

Pursuant the suggestion from @Wegatriespython at #523 (comment), this PR adjusts the GAMSInfo class to use shutil.which to locate the Python executable.

This may help overcome certain issues encountered when using ixmp in various ways, e.g. from terminals embedded in editors.

How to review

  • Install ixmp from this branch.
  • Run in your terminal: ixmp show-versions.
  • Run in the (I)Python command-line/notebook/REPL: import ixmp; ixmp.show_versions()
  • Try various situations:
    • With GAMS installed, or not.
    • With the IXMP_GAMS_PATH environment variable set, or not.
  • Report experience/results.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update release notes.

@khaeru khaeru self-assigned this Mar 25, 2025
This was linked to issues Mar 25, 2025
GAMS executable not found in PATH #456
Closed
GAMS is not registered correctly in show-versions #523
Closed
CalledProcessError: Command '['gams', …]' returned exit status 1 #563
Closed
@khaeru
Copy link
Member Author

khaeru commented Mar 25, 2025

@glatterf42 @behnam-zakeri @Wegatriespython @Tyler-lc —as reporters of/contributors to #456 #535 and/or #563, can I ask you each to please try out this PR branch and see if it helps address the respective issues?

If not, your precise and detailed reports will help to iterate until we have something that improves on the current code.

@codecov Codecov
Copy link

codecov bot commented Mar 25, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.4%. Comparing base (ca46768) to head (ea0058b).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files 
@@          Coverage Diff          @@
##            main    #564   +/-   ##
=====================================
  Coverage   98.3%   98.4%           
=====================================
  Files         50      50           
  Lines       5913    5935   +22     
=====================================
+ Hits        5818    5841   +23     
+ Misses        95      94    -1
Files with missing lines Coverage Δ
ixmp/model/gams.py 100.0% <100.0%> (ø)
ixmp/tests/test_model.py 100.0% <100.0%> (ø)
ixmp/util/__init__.py 98.8% <100.0%> (-0.1%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@khaeru
Copy link
Member Author

khaeru commented Mar 25, 2025

Not quite sure what's going on with Codecov here.

glatterf42
glatterf42 approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up :)

I didn't encounter this issue on my own system (yet), so not sure how well I can test this, but I'll try a few things.

ixmp/model/gams.py
Comment on lines 104 to +110
if match := re.search(r"^GAMS ([\d\.]+)\s*Copyright", output, re.MULTILINE):
self.version = match.group(1)
else: # pragma: no cover
self.version = None
else:
self.version = (
Copy link
Member

Choose a reason for hiding this comment

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

Something's wrong with codecov for sure, but we could cover line 107 at least by splitting this:

match = re.search(r"^GAMS ([\d\.]+)\s*Copyright", output, re.MULTILINE)
self.version = match.group(1) if match else ...

@glatterf42
Copy link
Member

As it stands, gams is installed on my system and added to the PATH. This is what I tried, then, after installing this branch in a fresh venv:

  • ixmp show-versions shows the correct version
  • In the Python console: ixmp.show_versions() shows the correct version
  • In the Python console in VSCode: ixmp.show_versions() shows the correct version
  • Remove gams from PATH; now I see only GAMS: no 'gams' executable in IXMP_GAMS_PATH=(not set) or the system PATH in the show-versions output
  • IXMP_GAMS_PATH=~/gams/gams49.1_linux_x64_64_sfx ixmp show-versions shows the correct version
  • Add gams back to PATH: everything is as before

So all this seems fine to me :)

khaeru added 2 commits April 28, 2025 15:21
@khaeru
Improve .model.gams.GAMSInfo
bd4a311 
- Add new utility attribute .executable.
  - Use shutil.which().
  - Handle an environment variable in IXMP_DATA_PATH.
- Expand tests.
- Reduce coverage exclusions.
@khaeru
Simplify usage of GAMSInfo in show_versions()
2a82d57
@khaeru khaeru merged commit 3726bb8 into main Apr 29, 2025
21 checks passed
@khaeru khaeru deleted the issue/456 branch April 29, 2025 13:58
@khaeru khaeru mentioned this pull request Jul 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glatterf42 glatterf42 glatterf42 approved these changes

Assignees

@khaeru khaeru

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@khaeru @glatterf42