astropy / saba

A Package which allows astropy to interface with sherpa
GNU General Public License v3.0
8 stars 9 forks source link

Fix data initalisation and APE17 #35

Open hamogu opened 4 years ago

hamogu commented 4 years ago

This PR is rebased on #33, so that it includes the fixes made there. In addition, it implements APE17, which requires major changes to the astropy templates, see

In particular, the text infrastructure now uses "tox". Since a lot of files needed to be touched, a few smaller changes are included here (e.g. the location of the intersphinx mapping for sherpa) in cases where the infrastructure had to be updated anyway. Note that the only changes in the actual saba code in this PR are from #33.

I try to address both (#33 and APE17) in one PR here, because he have a bit of a chicken-and-egg situation: The tests for #33 fail because the infrastructure has changed and the test for APE17 migration alone fail because #33 is not yet merged.

hamogu commented 4 years ago

@astrofrog @Cadair I'm trying out the current status of the migration guide on this repro. Saba is pretty simple (no compiled extensions), few requirements, only a few pages of docs, so I thought this would be an easy case to try.

However, I'm running into a problem with tox: https://travis-ci.org/astropy/saba/jobs/657429093?utm_medium=notification&utm_source=github_status

One of the saba dependencies is sherpa and unfortunately, sherpa does check during the install if numpy is available and fails if not. You could argue that that is an error on the sherpa side, but the way dependencies install is sometimes out of our control. I tried to explicitly list numpy in the requirements before sherpa (https://github.com/hamogu/saba/blob/APE17_rebase/setup.cfg#L20) but apparently that does not force tox to install it in that order (see failure in travis).

@astrofrog @Cadair: I thought I'll let you know and see if you have a simple idea to resolve this. If not, we can just continue to use conda on travis instead of tox, so don't waste time with detailed debugging, but I did want ot give you this feedback on how well the migration guide already works.

hamogu commented 4 years ago

cc @nocturnalastro This is also an attempt to prevent code-rot, here in our infrastructure. While we can certainly go with astropy-helpers a while longer, we will eventually have to migrate off it, because astropy will stop support astropy-helpers. So there is no hurry on this PR, I just wanted to get started and see how it does in Travis.

hamogu commented 4 years ago

@nocturnalastro: The migration tox and to remove astropy-helpers is complete as far as I can tell. The docs build locally, if I remove the plot directive. So we are now waiting to see what happens in astropy/astropy#10002 which you identified as a blocking issue in #33.

Let me know if you have a need to run this locally and run into local issues with tox. I just did all that (install tox-conda to make tox work within my local conda installation etc.) so it's fresh in my mind. I'll also try to learn more about the changes to astropy modelling to see if there is anything I can do to help that along or if I find an alternative route to astropy/astropy#10002.

However, I just want to say, if we need to require astropy>-=4.2.1 that's OK for me, even if it is a BIG jump in astropy dependency.

nocturnalastro commented 4 years ago

Thanks for the offer I'm using pipenv ATM so we will see how well it tox plays with it :)

10002 is up for review so we'll see how that goes. We could implement the same sort of logic ourselves if we want unblock this PR and then just take it out later once its in upstream. I don't mind either way. I'm off work ATM so happy to write the patch.

hamogu commented 4 years ago

@nocturnalastro Given how long it's been since we had a release, I'm not in a hurry and can wait at least a few days to see what we get from 10002.

If you have time to work on saba, #31 or #33 also could need a push to get them over the finish line (even if the tests will fail right now for the same reasons that the tests here fail).