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

Update Pyomo requirement to 6.8.0 release #1465

Merged
merged 11 commits into from
Aug 23, 2024

Conversation

blnicho
Copy link
Member

@blnicho blnicho commented Aug 8, 2024

Summary/Motivation:

Update the Pyomo requirement following the 6.8.0 release.

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.

@blnicho blnicho added the CI:run-integration triggers_workflow: Integration label Aug 8, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Aug 8, 2024
@ksbeattie ksbeattie added Priority:Normal Normal Priority Issue or PR Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Aug 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.36%. Comparing base (a1be331) to head (9b3ebb7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1465      +/-   ##
==========================================
- Coverage   76.36%   76.36%   -0.01%     
==========================================
  Files         394      394              
  Lines       65130    65130              
  Branches    14429    14429              
==========================================
- Hits        49737    49734       -3     
- Misses      12831    12835       +4     
+ Partials     2562     2561       -1     

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

@blnicho blnicho added the CI:run-integration triggers_workflow: Integration label Aug 20, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Aug 20, 2024
@blnicho blnicho changed the title Bumping the IDAES tag of Pyomo to check for issues Update Pyomo requirement to 6.8.0 release Aug 21, 2024
@blnicho
Copy link
Member Author

blnicho commented Aug 21, 2024

@lbianchi-lbl @ksbeattie this is ready to review and merge. I've updated the requirement to the 6.8.0 Pyomo release which we cut on Tuesday.

@blnicho blnicho added the CI:run-integration triggers_workflow: Integration label Aug 21, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Aug 21, 2024
Copy link
Member

@ksbeattie ksbeattie left a comment

Choose a reason for hiding this comment

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

LGTM

@lbianchi-lbl
Copy link
Contributor

It looks like a few of the integration checks (namely those where IDAES is installed in "user mode", i.e. not in editable mode) are failing, seemingly because of Pint:

image

I wonder if this is caused by the fact that we're pinning Pint to a previous version since #1438 (IIRC because of an incompatibility with the way dimensionless and/or unitless quantities are rendered).

I'm not entirely sure why there would be a difference with respect to the editable/developer mode, so I'll have to look more closely into this.

@blnicho
Copy link
Member Author

blnicho commented Aug 22, 2024

@lbianchi-lbl looks like more recent versions of Pint started supporting Python 3.9 again (see: hgrecco/pint#1974) so we can try unpinning Pint (or requiring >=0.24.1) and fixing the tests with the updated rendering

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Aug 23, 2024

@lbianchi-lbl looks like more recent versions of Pint started supporting Python 3.9 again (see: hgrecco/pint#1974) so we can try unpinning Pint (or requiring >=0.24.1) and fixing the tests with the updated rendering

Thanks for looking into it @blnicho. I think your proposal sounds good, and IIRC, the decision to set a maximum version was mostly due to version incompatibilities than the need to update the tests (although that will still need to happen, not only in this repository, but also in our dependencies). I'll try updating to pint >= 0.24.1 as you suggest and see how it goes; hopefully, only those tests involving the outputs we expect to be affected will fail.

@lbianchi-lbl
Copy link
Contributor

lbianchi-lbl commented Aug 23, 2024

There is an issue with the same Pint version constraint being specified in idaes-ui, which is causing the test failures. I'm not sure there are other good solutions other than having to create a patch release for idaes-ui.

In the meantime, I'll try to remove ui from the list of optional dependencies install idaes-ui from a PR branch with the changes (IDAES/idaes-ui#52) and see how that goes so that we can first address the Pint-specific issues here.

lbianchi-lbl added a commit to lbianchi-lbl/idaes-ui that referenced this pull request Aug 23, 2024
@lbianchi-lbl
Copy link
Contributor

I'm currently trying to understand why installing in dev mode causes NumPy v1 to be installed:

image

while, in the user-mode installation, NumPy 2.1.0 gets installed instead:

image

I imagine this might be caused by one of the dev dependencies having a numpy <2 constraint that causes this. I'm trying to reproduce this locally to try to figure out which one is doing that.

@lbianchi-lbl
Copy link
Contributor

It looks like the (and as far as I can tell, the only) package distribution requiring numpy < 2 is Tensorflow:

"pythondistribution[tensorflow]": {
    "version": "2.17.0",
    "requires": [
        "absl-py >=1.0.0",
        "astunparse >=1.6.0",
        "flatbuffers >=24.3.25",
        "gast !=0.5.0,!=0.5.1,!=0.5.2,>=0.2.1",
        "google-pasta >=0.1.1",
        "h5py >=3.10.0",
        "libclang >=13.0.0",
        "ml-dtypes <0.5.0,>=0.3.1",
        "opt-einsum >=2.3.2",
        "packaging",
        "protobuf !=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5,<5.0.0dev,>=3.20.3",
        "requests <3,>=2.21.0",
        "setuptools",
        "six >=1.12.0",
        "termcolor >=1.1.0",
        "typing-extensions >=3.6.6",
        "wrapt >=1.11.0",
        "grpcio <2.0,>=1.24.3",
        "tensorboard <2.18,>=2.17",
        "keras >=3.2.0",
        "tensorflow-intel ==2.17.0 ; platform_system == \"Windows\"",
        "tensorflow-io-gcs-filesystem >=0.23.1 ; python_version < \"3.12\"",
        "numpy <2.0.0,>=1.23.5 ; python_version <= \"3.11\"",
        "numpy <2.0.0,>=1.26.0 ; python_version >= \"3.12\"",
        "nvidia-cublas-cu12 ==12.3.4.1 ; extra == 'and-cuda'",
        "nvidia-cuda-cupti-cu12 ==12.3.101 ; extra == 'and-cuda'",
        "nvidia-cuda-nvcc-cu12 ==12.3.107 ; extra == 'and-cuda'",
        "nvidia-cuda-nvrtc-cu12 ==12.3.107 ; extra == 'and-cuda'",
        "nvidia-cuda-runtime-cu12 ==12.3.101 ; extra == 'and-cuda'",
        "nvidia-cudnn-cu12 ==8.9.7.29 ; extra == 'and-cuda'",
        "nvidia-cufft-cu12 ==11.0.12.1 ; extra == 'and-cuda'",
        "nvidia-curand-cu12 ==10.3.4.107 ; extra == 'and-cuda'",
        "nvidia-cusolver-cu12 ==11.5.4.101 ; extra == 'and-cuda'",
        "nvidia-cusparse-cu12 ==12.2.0.103 ; extra == 'and-cuda'",
        "nvidia-nccl-cu12 ==2.19.3 ; extra == 'and-cuda'",
        "nvidia-nvjitlink-cu12 ==12.3.101 ; extra == 'and-cuda'"
    ]
}

(this was generated for all dependencies of NumPy, i.e. all packages containing a numpy requirement, using a modified version of https://github.com/watertap-org/getinfo; full output here: dependenciesof-numpy.json)

As far as I can tell, the impact for us is:

  • We'll have to continue testing and working with NumPy v1 for developer installs, unless we split the current dev installation into two: one with Tensorflow (restricting us to NumPy v1) and one without (where NumPy v2 will be installed)
  • Even if we continue to use a single dev installation with Tensorflow, NumPy v2 will still get tested as part of the user-mode installation in the integration checks (which are currently failing)
  • Once we fix those failures, we should be able to test (and therefore support, to the extent that uses of the NumPy API in IDAES are exercised by our test suite) compatibility for both NumPy major versions (which is good!)

The NumPy failures mostly look straightforward to fix, so I'll try to do that now.

@lbianchi-lbl lbianchi-lbl merged commit 575953b into IDAES:main Aug 23, 2024
38 checks passed
lbianchi-lbl added a commit to lbianchi-lbl/idaes-ui that referenced this pull request Aug 23, 2024
lbianchi-lbl added a commit to IDAES/idaes-ui that referenced this pull request Aug 23, 2024
* Update supported/tested Python versions

* Update Pint requirement

* Use idaes-pse from IDAES/idaes-pse#1465 for testing

* Remove use of np.NINF

* Revert "Use idaes-pse from IDAES/idaes-pse#1465 for testing"

This reverts commit ee4345c.
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.

7 participants