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

Allow newest version of OpenMDAO #400

Merged
merged 50 commits into from
Feb 15, 2023
Merged

Conversation

eytanadler
Copy link
Collaborator

@eytanadler eytanadler commented Feb 11, 2023

Purpose

Removed the upper bound on the OpenMDAO version. Because there may be changes in new OpenMDAO versions (or other packages for that matter) that break parts of OpenAeroStruct, I also added automatic GitHub Actions testing twice a month. This will tell us if any changes in the newest package versions have caused errors in OpenAeroStruct.

I've also updated black and flake8 to use the MDO Lab actions, which involved reformatting OpenAeroStruct for the new version of black.

Do we want to update the lower bound on OpenMDAO? Version 3.2.0 is getting pretty old. We aren't testing intermediate versions of packages so we don't really know that older versions still work.

Expected time until merged

Within a couple weeks?

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

@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #400 (ceeb22a) into main (473232a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #400   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files          93       93           
  Lines        6009     6012    +3     
=======================================
+ Hits         5806     5809    +3     
  Misses        203      203           
Impacted Files Coverage Δ
openaerostruct/aerodynamics/lift_coeff_2D.py 100.00% <ø> (ø)
openaerostruct/aerodynamics/mesh_point_forces.py 100.00% <ø> (ø)
openaerostruct/aerodynamics/pg_scale.py 100.00% <ø> (ø)
openaerostruct/aerodynamics/viscous_drag.py 89.39% <ø> (ø)
openaerostruct/common/atmos_comp.py 100.00% <ø> (ø)
openaerostruct/functionals/breguet_range.py 100.00% <ø> (ø)
openaerostruct/functionals/center_of_gravity.py 100.00% <ø> (ø)
openaerostruct/functionals/equilibrium.py 100.00% <ø> (ø)
openaerostruct/functionals/total_lift_drag.py 100.00% <ø> (ø)
...rostruct/geometry/geometry_mesh_transformations.py 99.66% <ø> (ø)
... and 16 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kanekosh
Copy link
Contributor

Thanks @eytanadler , looks good to me!
I think we should also update the OM lower bound. Do we also want to test the oldest OM version in the Github Action by defining a matrix? E.g. Dymos does that: https://github.com/OpenMDAO/dymos/blob/master/.github/workflows/dymos_tests_workflow.yml

I personally wouldn't bother testing it because it doubles the build time, but I'm okay either way.

@eytanadler
Copy link
Collaborator Author

Thanks @eytanadler , looks good to me! I think we should also update the OM lower bound. Do we also want to test the oldest OM version in the Github Action by defining a matrix? E.g. Dymos does that: https://github.com/OpenMDAO/dymos/blob/master/.github/workflows/dymos_tests_workflow.yml

I personally wouldn't bother testing it because it doubles the build time, but I'm okay either way.

I'll add a test for older OM versions. I believe GHA runs the build matrix in parallel, so it shouldn't slow it down much. On second thought, I guess if the version works there's no point in updating it since leaving as is will enable pip to resolve versions more easily if there are dependency conflicts.

@eytanadler
Copy link
Collaborator Author

@kanekosh @bernardopacini I think this is finally ready for review. The GHA build file isn't the prettiest thing you'll ever see, but it does the job it needs to.

Copy link
Contributor

@kanekosh kanekosh left a comment

Choose a reason for hiding this comment

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

Thank you Eytan! Other than the pygeo version in setup.py, looks good to me

@@ -11,7 +11,7 @@
optional_dependencies = {
"docs": ["sphinx_mdolab_theme"],
"test": ["pytest", "pytest-cov", "coverage"],
"ffd": ["pygeo>=1.6.0"]
"ffd": ["pygeo>=1.6.0"],
Copy link
Contributor

Choose a reason for hiding this comment

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

pygeo>=1.12.2 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. pyGeo 1.12.2 was the latest that works with the old versions of the other packages, but older versions of pyGeo work too. I'll see if 1.6.0 works with the old build and if so leave that as the old build version.

Copy link

@bernardopacini bernardopacini left a comment

Choose a reason for hiding this comment

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

One minor comment, but overall looks good to me!

- latest
* - OpenVSP (optional)
- 3.27.1
- 3.27.1

Choose a reason for hiding this comment

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

Is this duplicate by mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is intentional since the OpenVSP version is 3.27.1 on both the oldest and latest GitHub Actions builds

@eytanadler eytanadler merged commit 71045b3 into mdolab:main Feb 15, 2023
@eytanadler eytanadler deleted the update_om branch February 15, 2023 20:51
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.

None yet

3 participants