-
Notifications
You must be signed in to change notification settings - Fork 23
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
Combine funtofem, pyfuntofem folder into funtofem #183
Combine funtofem, pyfuntofem folder into funtofem #183
Conversation
This PR addresses the Issue, #140, in which the TransferScheme is stored in a separate folder and are not part of pyfuntofem package so they can't be imported directly from conda. I also moved mphys from |
So is the idea that my existing MPhys scripts run fine as-is with this import: from funtofem.mphys import MeldBuilder Or do I need to change something? |
You need to change it to |
@A-CGray is that okay with you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need @A-CGray to approve since this directly affects you.
I'm getting this error when running
I definitely have numpy installed |
@A-CGray I recreated your error by starting a new conda environment in python 3.8 and python 3.10. Both versions gave me the same numpy error as you. Here's what I did to fix it, main thing was conda installing numpy. Can you try again?
|
@A-CGray I added the pyproject.toml to this pull request. I submitted a PR to mphys OpenMDAO/mphys#152, and waiting on word back from that. |
@sean-engelstad can we preserve the mphys subpackage in the import path i.e. Also it sounds like this will be a breaking change, which means it should follow with a version bump |
It's probably worth mentioning this on the mphys call so the other users are aware of this as well |
@timryanb I just moved the mphys folder from |
pyproject.toml
Outdated
#pyproject.toml | ||
[build-system] | ||
# Minimum requirements for the build system to execute. | ||
requires = ['setuptools>=45.0', 'wheel', 'cython>=0.29', 'oldest-supported-numpy', 'mpi4py==3.1.1'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this 'mpi4py>=3.1.1'? I have 3.1.4 and it tried to install 3.1.1 unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip will still likely install a temporary version of mpi4py during the build regardless @kejacobson. This is how pip handles building cython packages. In general we pin against older versions of mpi4py because they are always forward compatible if the users system mpi4py is newer. I'd recommend keeping the mpi4py version number as was, since we've seen issues in the past with pip trying to build codes against newer versions mpi4py than the user is running. This is the same reason we specify 'numpy-oldest' in the build section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I changed this back to mpi4py==3.1.1
then. I thought there was a reason that was the convention in TACS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it didn't seem forward compatible when I tried to build it. My pip failed when building mpi4py 3.1.1 with ==, but found my existing 3.1.4 with the >= change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. Do you have the failure message? Also what version of pip are you using?
@kejacobson I've changed the pyproject.toml file to |
You can also accomplish this by adding |
I decided to leave the mphys folder in the directory |
Brian and I have decided to instead combine all the python and cython code into the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sean-engelstad Can you change the first topic in the conversation to indicate the new import name?
@bburke38 I updated the main message block of the PR to say that we are merging into |
funtofem
andpyfuntofem
folders into thefuntofem
folder for consistency. This way the python module installed by anaconda is calledfuntofem
which matches the conda package name and the github repo name.from funtofem import TransferScheme
andfrom funtofem.mphys import MeldBuilder
, etc.pyfuntofem
such asfrom pyfuntofem.model import FUNtoFEMmodel
orfrom pyfuntofem.driver import FUNtoFEMnlbgs
are imported now withfrom funtofem.model import FUNtoFEMmodel
andfrom funtofem.driver import FUNtoFEMnlbgs