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

use setuptools_scm, fix packaging bugs, and update docs #278

Merged
merged 10 commits into from Mar 23, 2022
Merged

Conversation

asross
Copy link
Contributor

@asross asross commented Mar 5, 2022

This should resolve #277, #216, and #276, and hopefully make it so downloading/uploading pyqg to/from pypi just works (though there's a potential issue with pyfftw vs. np.fft which I'll detail below). I've tested it using testpypi but I'm holding off deploying to the main pypi repository until we've gotten this merged and updated whats-new.rst (PR to follow).

I also suspect I might definitely have an issue building the docs, but we'll see what happens when this PR triggers a new build I'll fix that soon. I think I've resolved this issue by just upgrading all of the doc dependencies.

One other potential issue is that we lose the flexibility we previously had around pyfftw vs. np.fftw. In order for plain old pip install pyqg to support pyfftw (which I think we should want as the default), we need to specify it in pyproject.toml. But, this means users have to download it to install or run pyqg (unless they want to install pyqg by cloning the repository and doing a local install), which makes it effectively a requirement (that also required updating CI to only run tests using it).

Another option is to omit pyfftw from pyproject.toml but tell pyfftw users to run pip install pyqg --no-build-isolation. That feels a bit wonky though.

@asross asross requested a review from rabernat March 5, 2022 15:47
@asross asross changed the title Use setuptools_scm, fix packaging bugs, and update docs WIP: use setuptools_scm, fix packaging bugs, and update docs Mar 5, 2022
@asross asross changed the title WIP: use setuptools_scm, fix packaging bugs, and update docs use setuptools_scm, fix packaging bugs, and update docs Mar 5, 2022
Copy link
Member

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Andrew thanks so much for working on this. I read through this issue:

pretty carefully and concluded that what we want--optional build dependencies--is not supported by pyproject.toml.

TBH this seems like a huge rabbit hole of complexity.

So for now, I am fine with simply requiring pyfftw as a hard dependency. Going forward, maybe we can find a way to make it optional again.

@rabernat rabernat merged commit 3b6b15c into master Mar 23, 2022
@rabernat rabernat mentioned this pull request Mar 23, 2022
@rabernat
Copy link
Member

rabernat commented Mar 23, 2022

After merging this, I just tried pip installing from git

% pip install git+https://github.com/pyqg/pyqg.git
Collecting git+https://github.com/pyqg/pyqg.git
  Cloning https://github.com/pyqg/pyqg.git to /private/var/folders/n8/63q49ms55wxcj_gfbtykwp5r0000gn/T/pip-req-build-l_3uvzg1
  Running command git clone --filter=blob:none --quiet https://github.com/pyqg/pyqg.git /private/var/folders/n8/63q49ms55wxcj_gfbtykwp5r0000gn/T/pip-req-build-l_3uvzg1
  Resolved https://github.com/pyqg/pyqg.git to commit 3b6b15cb282df9924a4be9ac2dd939c17290e8c0
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Collecting cython
  Using cached Cython-0.29.28-py2.py3-none-any.whl (983 kB)
Collecting numpy
  Using cached numpy-1.22.3-cp39-cp39-macosx_10_14_x86_64.whl (17.6 MB)
Building wheels for collected packages: pyqg
  Building wheel for pyqg (pyproject.toml) ... done
  Created wheel for pyqg: filename=pyqg-0.5.1.dev3+g3b6b15c-cp39-cp39-macosx_10_9_x86_64.whl size=161316 sha256=3b00d5b6d42c67864012d5be550418fd4aac13133374f2199e8081e45f7b07d4
  Stored in directory: /private/var/folders/n8/63q49ms55wxcj_gfbtykwp5r0000gn/T/pip-ephem-wheel-cache-kz7s0lvn/wheels/f3/13/1b/57f93b097bcabd35cfb3c25c9129a83588b3d19148c74e4c4f
Successfully built pyqg
Installing collected packages: numpy, cython, pyqg
Successfully installed cython-0.29.28 numpy-1.22.3 pyqg-0.5.1.dev3+g3b6b15c

I am puzzled by the fact that pyfftw was not installed.

And then it turns out that pyqg is not actually importable.

% python -c 'import pyqg'  
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/miniconda3/envs/pyqg_scratch/lib/python3.9/site-packages/pyqg/__init__.py", line 2, in <module>
    from .model import Model
  File "/opt/miniconda3/envs/pyqg_scratch/lib/python3.9/site-packages/pyqg/model.py", line 7, in <module>
    from .kernel import PseudoSpectralKernel, tendency_forward_euler, tendency_ab2, tendency_ab3
  File "pyqg/kernel.pyx", line 15, in init pyqg.kernel
ModuleNotFoundError: No module named 'pyfftw'

@asross
Copy link
Contributor Author

asross commented Mar 23, 2022

Definitely puzzling. From this issue, here's one hypothesis: maybe pip install for a git repository doesn't actually look at pyproject.toml (which is used in generating the dist/ files that would get installed in a pip install from pypi), but instead looks at the install_requires key of setup.py. For that particular installation command, we need to update install_requires to include pyfftw as well.

I can make a PR / branch which implements that change, and maybe pip installing via git from that branch will work?

@rabernat
Copy link
Member

Ah ok so you're saying it got the dependencies from here:

pyqg/setup.py

Lines 60 to 64 in 3b6b15c

### Dependency section ###
install_requires = [
'cython',
'numpy'
]

It feels a little redundant to have to specify both. I guess this is why people hate python packaging? 🙃

@asross
Copy link
Contributor Author

asross commented Mar 23, 2022

Ah ok so you're saying it got the dependencies from here:

pyqg/setup.py

Lines 60 to 64 in 3b6b15c

### Dependency section ###
install_requires = [
'cython',
'numpy'
]

It feels a little redundant to have to specify both. I guess this is why people hate python packaging? 🙃

A plausible hypothesis 🙃

@asross
Copy link
Contributor Author

asross commented Mar 23, 2022

pip install git+https://github.com/pyqg/pyqg.git@more-setup-debugging should hopefully work now?

@asross
Copy link
Contributor Author

asross commented Mar 23, 2022

(We might run into one more issue with numpy where setup requires a slightly older version numpy (1.20) even if the actual library is run with a later one -- if so we can update setuptools even further. So if that command doesn't work immediately I'll work on a more complete debugging later today or tomorrow.)

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.

Update version (+ on pip / Anaconda)
2 participants