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

Remove deprecations #362

Merged
merged 6 commits into from
Nov 20, 2023
Merged

Remove deprecations #362

merged 6 commits into from
Nov 20, 2023

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Nov 13, 2023

Purpose

Closes #361,
Closes #359.

Also updated the testflo flag since we now have IPOPT on Intel images.

Expected time until merged

1 week

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner November 13, 2023 02:51
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

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

Comparison is base (ccc40ca) 84.42% compared to head (d1b97aa) 73.38%.

Files Patch % Lines
pyoptsparse/types.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #362       +/-   ##
===========================================
- Coverage   84.42%   73.38%   -11.04%     
===========================================
  Files          22       22               
  Lines        3307     3307               
===========================================
- Hits         2792     2427      -365     
- Misses        515      880      +365     

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

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 13, 2023

I'm a little confused, maybe I'm too out of it now

@swryan
Copy link
Contributor

swryan commented Nov 14, 2023

This PR has been merged and a new release of testflo (1.4.14) has been pushed to pypi with the bug fix.

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 14, 2023

Think we may have to wait the full cycle for base/deps to rebuild.. why did we put some testing requirements in that layer anyways? Shouldn't they get picked up when we install pip install .[testing]? @eirikurj

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 15, 2023

This PR is ready @marcomangano @ArshSaja

@eirikurj
Copy link
Contributor

@nwu63 yeah, we need to wait for the deps images to be updated for an updated testflo. Ironically, we could use the feature in of https://github.com/mdolab/docker/pull/252 now, but tests are failing because it needs this PR.

why did we put some testing requirements in that layer anyways?

Primarily, since they are intended for testing we chose to install packages that are common to all downstream layers upfront. Aside from that I think at the time many package had an "incomplete" setup.py file, e.g., did not have testing entry or were missing packages. I think if we do not want to fix the testflo version in our docker images (enforced with the constraint file), then a floting version (installed as part of setup the first time it appears) is probaby fine. However, we need to do this in steps as I think many packages are still missing the pip install .[testing] in our build scripts.

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 15, 2023

I think explicitly listing the testing requirements is good regardless. I don't believe we are fixing the testflo version in the constraint file, but that is fine as long as the requirements are consistent across packages. In the most ideal case, we would be testing each package separately even in the nightly Docker cronjobs, so one package does not affect another downstream. Maybe that's discussion for another time.

Please approve/merge this soon as the nightly builds will start failing.

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Changes look good. Deprecation warnings are gone (tested with same case locally as #359). Did not explicitly test numpy 2.0 changes, but these are straight forward.

@eirikurj
Copy link
Contributor

@marcomangano @ArshSaja please review

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

I don't have the bandwidth to test this right now, but LGTM

@ArshSaja ArshSaja merged commit f56e159 into main Nov 20, 2023
12 of 13 checks passed
@sseraj sseraj deleted the remove-deprecations branch November 20, 2023 15:59
ewu63 added a commit to ewu63/pyoptsparse that referenced this pull request Dec 4, 2023
* np.float_ -> np.float64

* fix numpy 1.25 deprecations

* we now test everything

* revert change to fact

* trigger new build

* remove multi-objective test instead of skipping
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.

NumPy 1.25 deprecation NumPy 2.0 compatibility
5 participants