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

Set up scikit-build-core for the fortran extension module #99

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eguiraud
Copy link

@eguiraud eguiraud commented Mar 24, 2024

Replaces #96.
Fixes #98 .

@eguiraud eguiraud force-pushed the scikit-build branch 2 times, most recently from 9be388c to d53854d Compare March 24, 2024 01:44
It has issues producing an editable install of the Fortran extension,
and this system is on the way out anyways, with pyproject.toml and
related developments being the currently recommended way to do packaging.
Comment on lines +32 to +34
- name: Run test_basic.py
run: |
python examples/test_basic.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fsciortino let me know in case there are other scripts that would be better suited as CI tests

Copy link
Owner

Choose a reason for hiding this comment

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

@eguiraud in principle, all the "test_*" scripts were created with the idea of using pytest, but I only ever did that on the command line, rather than online with proper CI. Anyway, I think that using just test_basic.py would be enough -- it catches most of what we want to check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

test_basic.py in particular does not seem to be written as a pytest test: it has no test function inside, all instructions are at global scope, it does its own simple command line parsing...?

@@ -1,43 +0,0 @@
# Usage:
Copy link
Owner

Choose a reason for hiding this comment

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

@eguiraud I've always kept the makefile for "old fashion compatibility". Everyone who's used to work with Fortran is used to a makefile. While things like setup.py and pyproject.toml change over time, the makefile is the simplest and most robust thing... but if you think that it should be removed, then so be it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

make aurora compiles the fortran into a shared library, but it does not produce a working python environment (it does not install the dependencies, it does not check that the compiled shared object can be imported from python, etc.).

it would make sense to also have a makefile if aurora was also meant to be used as a standard fortran module: then if you want a python package you use pip install, if you want a fortran library you use make aurora. my understanding was that aurora is only meant to be used as a python package, so it should use normal python-packaging systems.

f2py3 -c --fcompiler=${fcompiler} -m _aurora aurora/main.f90 aurora/impden.f90 aurora/math.f90 aurora/grids.f90 --opt=${flags}
mv _aurora.cpython*.so aurora/

julia :
Copy link
Owner

Choose a reason for hiding this comment

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

If we are removing the makefile, we should eliminate all of the julia stuff from the package. I'm not opposed, since it's anyway broken. We should then remove it also from the documentation (I can take care of that).

Julia stuff appears in 3 places, I think

  • a .jl directory with actual julia code
  • in core.py, there is an interface between python and julia (crazy, I know)
  • in the docs, there must be some mention

I'm in favor of cleaning up, once and for all, since you're prompting me to do things properly...

Copy link
Collaborator

Choose a reason for hiding this comment

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

will do in a subsequent PR if that's ok?

# 2. because omfit_classes uses xarray.plot.plot, which has
# been removed/renamed by a commit from October 2022.
# This constrains xarray<=v2022.09.0
"xarray==v2022.09.0",
Copy link
Owner

Choose a reason for hiding this comment

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

ciao @orso82! Would you mind checking if this is what you'd recommend? @eguiraud went pretty deep into figuring out what is the issue. Since Aurora is used in many MFE integrated modeling frameworks at this point, I asked @eguiraud to help with setting up more robust CI for it. Are these changes for xarray and matplotlib what you'd recommend at the moment for omfit compatibility?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ciao! note that, ideally, just having omfit_classes in aurora's dependencies should transitively pick up omfit's dependencies and version constraints, but omfit does not advertise its dependencies afaict.

Copy link
Collaborator

Choose a reason for hiding this comment

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

P.S.
to reproduce the problems you can just pip install omfit_classes in a fresh virtual env. It does not bring in any of its dependencies.

After adding them manually with pip install uncertainties omas xarray matplotlib numpy you should be able to trigger the other issues by using related parts of omfit_classes (all problems are at import time, but omfit performs some imports lazily so you might have to invoke the related functions to see them, e.g. utils_math.contourPaths to see the problem with matplotlib._contour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi all, let me ping @wdeshazer @smithsp @kalling since they have been taking over most of the OMFIT support and are familiar with the OMFIT environments. From what I can tell the solution is to simply start using contourpy as already suggested here: https://github.com/gafusion/OMFIT-source/issues/6784

Choose a reason for hiding this comment

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

I have a proposal on the fix. I guess I could prioritize that since it seems to be impacting a lot of people.
https://github.com/gafusion/OMFIT-source/issues/6966

Copy link

Choose a reason for hiding this comment

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

@eguiraud Thank you for being thorough in figuring out what aurora needs. That is helpful in guiding OMFIT development.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @smithsp , @orso82 ,

I am sure there is a lot of context that I am missing, a lot of history I am not aware of, etc, so forgive me if I sound naive or even, perish the thought, entitled. just as an outsider perspective that might be useful input: I think it's completely non-controversial to expect that you pick one between not advertising your requirements and publishing the package on pypi and conda -- the way things are right now omfit_classes, as a pypi/conda package, is broken, because pip install omfit_classes yields a broken environment.

Copy link
Collaborator

@orso82 orso82 Apr 15, 2024

Choose a reason for hiding this comment

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

@eguiraud the OMFIT classes have essentially the same dependencies of the OMFIT framework. Here the required dependencies are needed for OMFIT to start, while the optional dependencies are needed for some imports done in some specific class.

One way would be to generate a requirements.txt file with all of those dependencies, like this:

black>=19.3b0                    #{'REQUIRED':'Format Python code within OMFIT'}
configobj>=5.0                   #{'REQUIRED':'Used to parse input files for IPS'}
dill>=0.2.7                      #{'REQUIRED':'Needed for saving projects with chaospy distribution objects in them'}
fortranformat>=0.2               #{'REQUIRED':'OMFIT needs to read and write ascii format files similar to FORTRAN: afiles, gfiles, etc.'}
h5py>=2.7                        #{'REQUIRED':'hdf5 files for reproducing plots; NIMROD, FIDASIM and COMPASS modules'}
jedi>=0.17.1                     #{'REQUIRED':'Tab completion in command box'}
lmfit>=0.9,!=1.0.1               #{'REQUIRED':'Profile fitting, STRAHL minimization'}
matplotlib>=3.1,!=3.2.1,!=3.2.2  #{'REQUIRED':'Expansive plotting capabilities'}
netCDF4>=1.3.1                   #{'REQUIRED':'Many fusion codes output results in netcdf format'}
numpy>=1.12                      #{'REQUIRED':'Fast array operations'}
pexpect>=3.3                     #{'REQUIRED':'Passwordless login and TRANSP setup'}
psutil>=5.4                      #{'REQUIRED':'to get OMFIT memory usage'}
pygacode>=0.57                   #{'REQUIRED':'Needed to interface with GACODE'}
pyodbc>=4.0                      #{'REQUIRED':'Fusion data is stored in SQL databases'}
pyotp>=2.2                       #{'REQUIRED':'Provides one-time-password functionality'}
pyttk>=0.3                       #{'REQUIRED':'OMFIT makes use of some pyttk features not available with Tkinter'}
pyyaml>=3.13                     #{'REQUIRED':'YAML files used for generating OMFIT install-scripts'}
requests>=2.20                   #{'REQUIRED':'Used to track TRANSP runs for JET and GitHub API interactions'}
scipy>=1.0                       #{'REQUIRED':'Access physical constants, interpolation, integration, etc.'}
tqdm>=4                          #{'REQUIRED':'Used for displaying progress bar'}
uncertainties>=3                 #{'REQUIRED':'OMFITprofiles; ONETWO; MDS+'}
xarray>=0.10.8                   #{'REQUIRED':'OMFITprofiles, BES; Smart dimensioning'}


aurorafusion>=2.1                #{'optional':'Used in ImpRad, STEP and CHEF modules'}
bibtexparser>=0.6                #{'optional':'Keep track of papers that used OMFIT; also needed for omfit_bibtex class'}
boto3>=1.5                       #{'optional':'Download harvest objects stored on amazon servers'}
botocore>=1.8                    #{'optional':'Download harvest objects stored on amazon servers'}
chaospy>=3.0.11,<=3.0.12         #{'optional':'Used to construct proxy functions for TGLF UQ'}
corner>=2.0,<=2.1.0              #{'optional':'Make some beautiful corner plots'}
csaps>=0.11                      #{'optional':'Needed for profile fitting in CAKE'}
Cython>=0.28                     #{'optional':'An optimising static compiler for the Python and Cython languages'}
dask>=0.17                       #{'optional':'Needed for stable xarray import'}
dicttoxml>=1.7                   #{'optional':'EU/IM actors'}
emcee>=2.2                       #{'optional':'Gaussian processes fitting'}
ffmpeg>=1.4                      #{'optional':'Used to make animations from matplotlib'}
gptools>=0.2                     #{'optional':'Used in OMFITprofiles for profile fitting'}
gpr1dfusion>=1.0                 #{'optional':'gpr1dfusion'}
imbalanced-learn>=0.3            #{'optional':'imbalanced-learn'}
ipython>=5.4                     #{'optional':'ipython'}
matplotlib-label-lines>=0.3.9    #{'optional':'Better MARS-F eigenfunction plots'}
mayavi>=4.5                      #{'optional':'Make good 3d graphs; GPEC, M3D-C1, TRIP3D modules'}
nlopt>=2.4                       #{'optional':'to offer wide range of nonlinear optimizers'}
numba>=0.38                      #{'optional':'speed up numerically intensive python code'}
oyaml>=1                         #{'optional':'OMFIT uses yaml files in several places and it is nice to be able to maintain dict ordering.'}
pidly>=0.2                       #{'optional':'Start an interactive IDL<->python session; there are many fusion analysis codes already written in IDL'}
psycopg2-binary>=2.7.3           #{'optional':'Osborne pedestal tools and OMFIT shape control store settings in postgresql databases'}
pyAesCrypt>=6.0.0                #{'optional':'used for password-based encryption/decryption'}
pygsheets>=2.0.3.1               #{'optional':'Used for interfacing with Google sheets in omfit_google_sheet.py'}
PyPDF2>=1.26                     #{'optional':'Allow embedding of files in PDFs'}
tables>=3.4                      #{'optional':'reading fast camera .cine files'}
python-dateutil>=1.5             #{'optional':'Used for OMFIT statistics'}
scikit-image>=0.13               #{'optional':'Used required by the python fidasim module'}
scikit-learn>=0.24               #{'optional':'Used by several modules including KN1D and TGLF_scan'}
scikit-optimize>=0.5             #{'optional':'Used for machine learning'}
scikit-sparse>=0.4               #{'optional':'Used for profile fitting'}
seaborn>=0.8                     #{'optional':'SCOPE, M3D-C1, TUTORIAL modules'}
Shapely>=0.6                     #{'optional':'Find intersections of radiated power sightlines and first wall in PCS prad control module; Also needed in eqdsk utils'}
sphinx>=1.6                      #{'optional':'producing OMFIT documentation'}
sphinx_bootstrap_theme>=0.5      #{'optional':'producing OMFIT documentation'}
sphinxcontrib-bibtex>=1.0        #{'optional':'Producing GACODE documentation'}
tensorflow<=2.6                  #{'optional':'Leverage Google advancements in machine learning'}
twine>=3                         #{'optional':'Used for uploading standalone OMFIT classes to pypi'}
unbaffeld==0.2.2                 #{'optional':'Used for GPR fitting in OMFITprofiles'}
wordcloud>=1.8                   #{'optional':'OMFITstats module for module usage visualization'}
xmltodict>=0.11                  #{'optional':'OMFITxml'}

However, I felt this may be an overkill for someone who wants to use only few specific OMFIT classes. Hence I decided that omfit_classes will have no explicit dependency, and will let individuals figure out what packages are needed for the classes they were interested in using. In other words, once you figure out what packages you need to run the specific omfit_classes that you want, then you can add those packages as explicit dependency of Aurora.

An alternative approach that I pursued at some point, but then abandoned, was to make a PyPi package for each of the OMFIT classes. I found that to be overly complicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @orso82 , some comments in mostly random order:

  • imagine if 5 or 6 packages that omfit_classes depends on adopted this same approach and omfit_classes had to manually list and keep up to date the list of dependencies (of dependencies, of dependencies, ...) of its dependencies. this unique approach puts a burden on packages that depend on omfit_classes in a way that, if more packages than omfit_classes adopted the same approach, the python software ecosystem would basically be constantly broken. luckily, the norm is that if you pip install x, you get an environment with x and compatible versions of its dependencies, so you can use x :D

  • will let individuals figure out what packages are needed for the classes they were interested in using

    how should those individuals figure out the list (other than pinging you from github PRs like we are doing here, which does not scale)? and how would they know to update that list of dependencies as needed when they switch to a new omfit_classes versions? EDIT: I found that list at https://omfit.io/install.html#setup-the-omfit-python-environment, so never mind this question, but if you will ever have more than one version of omfit_classes in flight (e.g. a stable version and an older version that's still supported) you will need separate such lists

  • there is a standard way to deal with optional dependencies: https://packaging.python.org/en/latest/specifications/dependency-specifiers/#extras. omfit_classes can document which parts of the package need what extras, and downstream packages can then pip install omfit_classes[omfitxml,omfit-statistics]. they will only get the optional transitive dependencies that are needed for those extra pieces of omfit_classes

  • as per my comment above about xarray and matplotlib compatibility, the requirements.txt that you posted is too liberal w.r.t. the versions of these packages and might yield broken environments

  • I see aurorafusion is an optional dependency of omfit_classes, creating a circular dependency between the two packages. although I think pip and other python package managers can deal with circular install dependencies, Python cannot handle circular imports, so this is something to be aware of (@fsciortino FYI)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eguiraud I agree with all of what you say.

You raise a good point with the optional dependence approach. I like it. We don't have a way to identify dependencies on a per-class basis... this will have to be done manually.

Unfortunately until that's done, you'll have to explicitly add the dependencies of that omfit_classes does not advertise.

@odstrcilt
Copy link
Collaborator

@fsciortino do you think that we should remove the omfit_classes dependence?
Most of the time, it uses these functions:

omfit_classes.omfit_eqdsk
omfit_classes.math_utils.atomic_element
omfit_classes.omfit_mds

I have some script for parsing eqdsk files. Several Aurora users have already contacted me that Omfit is not loading their eqdsk files properly so that we can try another option. omfit_mds can also be easily replaced, but it will not support remote SSH tunneling to DIII-D. atomic_element should be also easy.

Then there are a few other uses; I'm not sure how difficult it can be:

aurora\oedge.py:        from omfit_classes.utils_base import tolist
aurora\oedge.py:        # import omfit_classes here to prevent import during regression tests
aurora\oedge.py:        from omfit_classes.omfit_nc import OMFITnc
aurora\oedge.py:            from omfit_classes import utils_math
aurora\oedge.py:    from omfit_classes.omfit_namelist import namelist
aurora\solps.py:            from omfit_classes import omfit_solps

@fsciortino
Copy link
Owner

@odstrcilt the intention in using omfit_classes in the first place was to avoid replication of work and staying up to date with file formats, joining the OMFIT community in a joint effort. I still think that this is a good strategy, and probably one that facilitates integrated modeling especially at GA and elsewhere.

I see that @wdeshazer has been doing some deep work on this last week, which is much appreciated. If he were able to resolve the omfit_classes incompatibilities over the upcoming week, I would be in favor of keeping the dependency. However, the need for effective CI/CD for Aurora has become really acute recently, so I would appreciate this being considered as a priority.

@wdeshazer
Copy link

wdeshazer commented Apr 7, 2024

@fsciortino I am definitely aware of your need and getting to a Python 3.8 or higher is my top priority. I don't have a way to get it it into unstable by the end of the week because I am confident there will be issues beyond getting an updated environment. Would you be amenable to working on a development branch that could get you moving with the contourpy fix that you require? Also, what is your need for MDS+ access? That is one of the major show stoppers and up to now, it has been out of my control. We need support of the MDS+ support at MIT and Sterling has assigned issues related to getting DIII-D startup issues as higher priority.

If you could do without MDS+ we might be able to do a skinny build that could also keep you moving.

All in all, your message is received and I will keep you informed.

Note

If you wanted to discuss over zoom to see what is going on and discuss options I would be happy to. Just drop me a personal note on Discord, Zoom or Email with your availability and I will work around it.

@wdeshazer
Copy link

@fsciortino I want to give you an update. I have been working this much of the week and most of the weekend. Where we stand is that MDSPlus is an essential package and we can get successful builds on the linux platforms but not the macOS. The new M2 Arm chips require an arm64 build, but many dependencies are misidentifying the platform and performing builds that are appropriate for the architecture when they switched from Intel to Arm chips. Interestingly we had one successful build that I only today realized was because I was using the Homebrew mamba accidentally. I am going to try it now that I think I have an explanation. I don't know if that will solve our problmes, because I don't know that we can pass it back to conda-forge, but it could give us some insight.

When we make the break-though that I hope will come shortly, then I will work through the rest of the OMFIT-core-deps, which were all looking positive. If we get OMFIT-core-deps building successfully, we have the heart of OMFIT, we can work though the optional dependencies by removing dependencies that we can live without until we have time to solve them. I don't have a time-line on when this will be done, because we are needing a break-through and/or support from MIT, who are prioritzing Operational Readiness over this.

As I have offered before, feel free to reach out if you want to discuss ir more, and I would be happy to support any stop-gap solutions that we can come up with to hopefully get your tasks advancing.

Good luck,

Will

@fsciortino
Copy link
Owner

Hi @wdeshazer , thanks for the update. I greatly appreciate that you're trying your best, and realize that it's not an easy task. Would it help if I reached out to the MDSplus group and insisted a bit more on their support? If so, please share the email thread that you have with them at my firstname.lastname @ ipp.mpg.de email address (to which I still have access).

@wdeshazer
Copy link

@fsciortino Thank you for the offer, but they are working through @smithsp who directed them to prioritize Operational Readiness. So, they are already doing as requested.
I will keep working. I am hopeful we are close.

@smithsp
Copy link

smithsp commented Apr 18, 2024

@fsciortino The thread with MDSplus is MDSplus/mdsplus#2736 . You can comment on that as you will.

@wdeshazer did get the mdsplus osx build uploaded to his channel on anaconda.org . He is working through a few other dependencies. The next step after that will be to test that new environment, where we already know there will be some issues if we try to move to the latest versions of packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable testing of editable installs
7 participants