search

astropy / pyregion

ds9 region parser for python
https://pyregion.readthedocs.io
MIT License
39 stars 38 forks source link

MNT: fix CI and fix install on Linux #161

Closed jdavies-st closed 1 year ago

jdavies-st commented 1 year ago

This PR does 2 things:

  1. Fixes CI
  2. 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. https://github.com/jdavies-st/pyregion/pull/1

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

These 2 simple things above involve

  1. Tearing out astropy-helpers, which means modifying the build system and sphinx config to work without it.
  2. 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.
  3. 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 commented 1 year ago

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. 🤞

jdavies-st commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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()).

smaret commented 1 year ago

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

pllim commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Great idea, @prajwel ! I opened https://github.com/astropy/regions/issues/488