MNT: fix CI and fix install on Linux #161

jdavies-st · 2022-11-16T17:59:30Z

This PR does 2 things:

Fixes CI
Fixes the build/install on Linux, which is currently broken

I tested it on my own fork against main, and you can see the Github actions CI running there and the build and test works. jdavies-st#1

Running the workflows on this PR will need to be enabled by a maintainer. @pllim ?

These 2 simple things above involve

Tearing out astropy-helpers, which means modifying the build system and sphinx config to work without it.
Using modern PEP517 build tools such as pyproject.toml, build, setuptools-scm, etc. This project uses Cython and numpy as build dependencies, so of course these modern tools make everything just work.
Fix up any problems with the doc build or tests exposed by the CI.

Resolves #121
Resolves #139
Resolves #140
Resolves #147
Resolves #148
Resolves #150
Resolves #151
Resolves #156
Resolves #158
Resolves #160

Closes #144
Closes #149
Closes #155
Closes #157

pllim · 2022-11-16T20:03:28Z

Running the workflows on this PR will need to be enabled by a maintainer

It won't run here until after this PR is merged, so we'll have to trust the run from your fork. 🤞

pllim

Overall, I love the +302 −1,662 clean-up. Thanks!

I guess you don't want to be bothered with tox here? Just wondering if you are aware of https://github.com/OpenAstronomy/github-actions-workflows and would like to use that to further simplify your CI or not. If you don't want to do that now, we can always defer too.

Hard to tell if RTD would build or not until after merge.

If I am reading the diff correctly, you completely gutted pyregion/conftest.py, is that intentional?

Why remove pyregion/tests/__init__.py?

.github/workflows/ci.yml

MANIFEST.in

docs/conf.py

docs/installation.rst

pllim · 2022-11-16T20:12:19Z

docs/installation.rst

-`wcsaxes <https://github.com/astrofrog/wcsaxes>`__
-
-To work with the development version, you'll need Cython and a C compiler,
+To work with the development version, you'll need a C compiler,


You removed Cython from the text here but I see it in pyproject.toml build requirements, why the inconsistency?

One doesn't actually need Cython installed anymore to build the package. It gets pulled automatically to do the build now because it is PEP517-compliant, and is listed in pyproject.toml. Best not to confuse people. Just tell them how to install the package and leave out the underlying details of what happens under the hood.

pllim · 2022-11-16T20:24:34Z

pyregion/__init__.py

-# Affiliated packages may add whatever they like to this file, but
-# should keep this content at the top.
-# ----------------------------------------------------------------------------
-from ._astropy_init import *


So you don't need the test runner from _astropy_init?

No, just normal pytest for testing.

I mean, you removed a user's ability to test the installed package by running import pyregion; pyregion.test(). Is that intentional?

Yes, intentional. Is there a reason to support this very non-standard way of running tests?

If there is, I can put it back, but I think it's very much a legacy piece of infrastructure that is not common or recommended in python outside of astropy.

I do find it nice that user can run tests on their installed copy. I still call that myself when I publish though I guess I don't have to but it is just so convenient.

https://github.com/spacetelescope/synphot_refactor/blob/cb3ff4b05ae5f20419b5b4a781eac9496f24936c/.github/workflows/publish-to-pypi.yml#L38

Here, you are actually removing a feature, so should get okay from @larrybradley or @keflavich first.

Running tests on installed copies is very easy:

pip install pyregion[test] pytest --pyargs pyregion

And is the recommend way. I even updated the docs to show this is the standard way. Doing it this way ensures test dependencies are installed, and it works generally for python packages.

One can also run tests on the installed version if you are in the git repo checkout doing development:

pip install .[test] pytest --pyargs pyregion --import-mode=importlib

Which will install the package in site-packages and then run the tests from in there as well, ensuring not to add . to sys.path, i.e. accidentally import the code from the local checkout. --import-mode=append would also work if there a bunch of test helpers in the test dirs, which there are not in this package.

Ah, so pytest --pyargs pyregion can detect the installed copy even if you didn't check out the source or not in the site-packages dir? I didn't know that. pytest sure has come a long way since that time the whole thing was in one file that astropy bundles.

Yeah, you just have to use --import-mode set to anything other than the default, which is prepend. This can also be set in setup.cfg if needed, but it's nice to allow flexibility I think.

pyregion/tests/test_cube.py

larrybradley · 2022-11-17T14:31:09Z

.github/workflows/ci.yml

+    runs-on: ${{ matrix.os }}
+    strategy:
+      matrix:
+        python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"]


Can we drop 3.7?

We can! Though it is still officially supported by python.org, though that will change in mid-2023.

I think most packages in the python scientific stack have adopted NEP 29 (https://numpy.org/neps/nep-0029-deprecation_policy.html).

That makes sense. Given that I have no plans to add features or anything to this package, only to fix the build/install and verify it via CI, then I'm happy to have a wider allowed version schedule. Especially given that whatever code still uses it is probably also old. Of course if it stops working on Python 3.7 or Numpy 1.19 next year, happy to immediately drop support. I just didn't want to artificially bump allowed versions of dependencies without having a good reason for it.

I think dropping Python 3.7 can be a separate PR if needed, especially given it is still green here.

larrybradley · 2022-11-17T14:31:28Z

.github/workflows/ci.yml

+        python-version: ["3.8", "3.9", "3.10", "3.11"]
+        os: [ubuntu-latest, macos-latest, windows-latest]
+        include:
+          - python-version: "3.7"


Here I'm just building the oldest dependencies listed in install_requires on python 3.7, and it was to figure out what minimum version to specify, as there was none specified before.

larrybradley

Thanks @jdavies-st.

Just curious - what features do you need from this package?

jdavies-st · 2022-11-17T15:10:50Z

Overall, I love the +302 −1,662 clean-up. Thanks!

I guess you don't want to be bothered with tox here? Just wondering if you are aware of https://github.com/OpenAstronomy/github-actions-workflows and would like to use that to further simplify your CI or not. If you don't want to do that now, we can always defer too.

Yes, I figured the minimal "getting CI working" would be what I have here. Happy to implement tox and the OpenAstronomy workflow in a future PR if that's wanted. I'd prefer to defer to later on this.

Hard to tell if RTD would build or not until after merge.

It does! Though I don't know how the rtd builds are triggered on this repo. Maybe they're not? It seems the current docs are a bit behind the current release, and there was no rtd yml file in here before. A mystery. I did make sure that every page is identical to what is currently there.

If I am reading the diff correctly, you completely gutted pyregion/conftest.py, is that intentional?

Yes, though I realize I can include pytest-astropy-header and get that stuff back. I've done that.

Why remove pyregion/tests/__init__.py?

Not necessary, as tests is not an importable package.

jdavies-st · 2022-11-17T15:16:16Z

Just curious - what features do you need from this package?

grizli depends on it. And that is the workhorse package for all science being done with JWST grism data right now. The official jwst pipeline is not feature-complete for grism. And currently pyregion cannot be listed in install_requires for grizli because it cannot by pip installed on Linux. So one has to separately install it from source. Irritating for grizli CI and installation.

I figured fixing up the installation for pyregion would be much easier than trying to port over grizli to the new regions API, though that would be a nice project to do at some point as well. And this extends the lifetime of this package as well, given that regions is not feature complete yet.

pllim · 2022-11-17T20:35:00Z

Though I don't know how the rtd builds are triggered on this repo.

Well, the admin from https://readthedocs.org/projects/pyregion/ (@astrofrog) would need to enable https://docs.readthedocs.io/en/stable/pull-requests.html for RTD to build for PRs. Otherwise, we'll find out when this is merged...

tests is not an importable package.

You will have to add that __init__.py back if you want the test runner back (i.e., pyregion.test()).

pllim

Overall LGTM if you really don't think you need pyregion.test() anymore. And we can always fix up any remaining small things as follow-up PRs.

I'll approve but let the maintainers merge (@larrybradley or @keflavich).

larrybradley

Thanks, @jdavies-st.

smaret · 2022-11-22T11:35:21Z

I note that this partially duplicates the work in #157 which was opened two months ago and was never reviewed.

pllim · 2022-11-22T15:03:20Z

We apologize for the oversight, @smaret ! This package is minimally maintained. For future contributions, please contact other Astropy Project maintainers if you do not hear back. We have a several places for communication: https://www.astropy.org/help.html

prajwel · 2022-11-24T09:09:16Z

I knew this was just a matter of time. Pyregion is part of many packages due to its age. Perhaps we can create easy porting guidelines for people wanting to port their code to Regions 🤔?

pllim · 2022-11-30T19:28:17Z

Great idea, @prajwel ! I opened astropy/regions#488

jdavies-st added 12 commits November 15, 2022 14:56

Add CI workflow in github actions

34aafc4

Add pyproject.toml; clean setup.py

f4d2a23

Fix cython

b67fd86

Tests need abs paths

6e64e86

More fixes

eda6d70

plot directive

7e190b6

Add build job to CI

8989447

Add workflow_dispatch and cron to CI triggers

b414189

Add rtd config

89d6f3a

Fix doc build; update install instructions

2d1b7c3

Fixup docs

44cbad1

Add oldest dependencies CI job

9934068

pllim reviewed Nov 16, 2022

View reviewed changes

Add Windows runners to CI

e2af7e5

jdavies-st force-pushed the add-github-actions branch 2 times, most recently from bde434c to 7e77559 Compare November 17, 2022 12:17

Use --pyargs in CI

55cb7cf

jdavies-st force-pushed the add-github-actions branch from 7e77559 to 55cb7cf Compare November 17, 2022 12:39

jdavies-st added 3 commits November 17, 2022 14:30

Make sphinx nitpicky

b0be7d6

Add setup.py and pyproject.toml to MANIFEST.in

51f94ed

Fix typo in test, see issue astropy#121

7dae916

larrybradley reviewed Nov 17, 2022

View reviewed changes

Use pytest-astropy-header

6b9b87b

pllim approved these changes Nov 17, 2022

View reviewed changes

larrybradley approved these changes Nov 21, 2022

View reviewed changes

larrybradley merged commit 98fefa9 into astropy:main Nov 21, 2022

pllim mentioned this pull request Nov 30, 2022

DOC: Conversion guide from pyregion astropy/regions#488

Open

fabaff mentioned this pull request Feb 9, 2023

python311Packages.pyregion: skip failing tests NixOS/nixpkgs#215426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT: fix CI and fix install on Linux #161

MNT: fix CI and fix install on Linux #161

jdavies-st commented Nov 16, 2022 •

edited

Loading

pllim commented Nov 16, 2022

pllim left a comment

pllim Nov 16, 2022

jdavies-st Nov 17, 2022

pllim Nov 16, 2022

jdavies-st Nov 17, 2022

pllim Nov 17, 2022

jdavies-st Nov 18, 2022

pllim Nov 18, 2022

jdavies-st Nov 18, 2022 •

edited

Loading

pllim Nov 18, 2022

jdavies-st Nov 18, 2022

larrybradley Nov 17, 2022

jdavies-st Nov 17, 2022

larrybradley Nov 17, 2022

jdavies-st Nov 17, 2022

pllim Nov 17, 2022

larrybradley Nov 17, 2022

jdavies-st Nov 17, 2022 •

edited

Loading

larrybradley left a comment

jdavies-st commented Nov 17, 2022

jdavies-st commented Nov 17, 2022

pllim commented Nov 17, 2022

pllim left a comment

larrybradley left a comment

smaret commented Nov 22, 2022

pllim commented Nov 22, 2022

prajwel commented Nov 24, 2022 •

edited

Loading

pllim commented Nov 30, 2022

MNT: fix CI and fix install on Linux #161

MNT: fix CI and fix install on Linux #161

Conversation

jdavies-st commented Nov 16, 2022 • edited Loading

pllim commented Nov 16, 2022

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdavies-st Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdavies-st Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

larrybradley left a comment

Choose a reason for hiding this comment

jdavies-st commented Nov 17, 2022

jdavies-st commented Nov 17, 2022

pllim commented Nov 17, 2022

pllim left a comment

Choose a reason for hiding this comment

larrybradley left a comment

Choose a reason for hiding this comment

smaret commented Nov 22, 2022

pllim commented Nov 22, 2022

prajwel commented Nov 24, 2022 • edited Loading

pllim commented Nov 30, 2022

jdavies-st commented Nov 16, 2022 •

edited

Loading

jdavies-st Nov 18, 2022 •

edited

Loading

jdavies-st Nov 17, 2022 •

edited

Loading

prajwel commented Nov 24, 2022 •

edited

Loading