Deprecation warnings and test failures under Python 3.8

[dhomeier]

Two more minor fixes for DeprecationWarnings appearing with 3.8b1:

First is the

DeprecationWarning: PY_SSIZE_T_CLEAN will be required for '#' formats

from the votable.tablewriter call in io/votable/tree.py, causing most votable tests to fail.
Fixed by defining PY_SSIZE_T_CLEAN.

The second is a

def test_velref():
        w = _wcs.Wcsprm()
        assert w.velref == 0.0
>       w.velref = 42.0
E       DeprecationWarning: an integer is required (got type float).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.

as _wcs.Wcsprm.velref is an int, trivially fixed by setting it with an integer (raises the question whether the results should be compared to integers as well) - unless we wish velref to accept floats, of course (or rather want to filter the warning, as the removal notice is still vague).

Tested for backwards compatibility with Python 3.6.

[pllim]

The votable portion looks fine to me.

I'll defer to @nden for the wcs part.

[pllim]

@bsipocz , does Python 3.8 stuff get backported? How far back?

EDIT: Nvm... Saw your milestone. LOL

[bsipocz]

I'm on the side to backport these kind of fixes while they are small an painless, and draw the line when we run into some uncomfortable situations. I don't think we should aim for LTS, but 3.2 may still make some sense.

[dhomeier]

Forgot that we still support Python 3.5 (or rather confused it with matplotlib 3.1) - tested with 3.5.7 under OS X 10.12 as well now. I could not reproduce any of the failures on circleci 32bit, but note that my local tests were against this PR + @astrofrog's fast_reader fix #8851 on top of the 3.2.1 release only.

[nden]

The wcs change looks fine too. velref is defined as integer.

[bsipocz]

This doesn't exactly have the same failures as on master for 32bit, so I would hold up merging until at least master is sorted out.

[dhomeier]

How can one access the CI output for HEAD on master?
Every single failure on 32bit seems to be related to a failure checking for the error message in the ExceptionInfo object on that system, but I could only compare to the test results for other recent PRs (e.g. #8939 has three more, but no obvious differences in the actual test behaviour).
But indeed as noted in #8934 other PRs rather seem to raise 96 exceptions, not 93.

[bsipocz]

on circleCI you can select the master branch on the left hand side, on travis select the "build history" tab.

[dhomeier]

Ah, thanks, found it (in my browser it's on top).

[bsipocz]

@dhomeier - please rebase

[dhomeier]

Tried to exactly follow the manual in rebasing this time; still looks messy, sorry!
#8949 at least seems to have resolved the failures on ci/circleci 32bit.

[bsipocz]

Hmm, as I see you've rebased it on the master of your fork rather than on astropy master. This is basically the main reason why we suggest to remove the master branch of the fork :)

This is how I would do it:

git remote -v

astropy	git@github.com:astropy/astropy.git (fetch)
astropy	git@github.com:astropy/astropy.git (push)
dhomeier	git@github.com:dhomeier/astropy (fetch)
dhomeier	git@github.com:dhomeier/astropy (push)

git fetch --all

make sure you're on the branch:

git checkout python-38-ssize_t+int-fix

git rebase -i astropy/master

This is a clear case, so the interactivity is not even needed, but nevertheless it's good to get into the habit of using the interactive way. So, now you have an editor window with the 2 commits listed. Save and exit it.

And finally the force push:
git push dhomeier python-38-ssize_t+int-fix --force

[dhomeier]

Hmm,

> git remote -v show                                                                                                                   
origin	git@github.com:dhomeier/astropy.git (fetch)
origin	git@github.com:dhomeier/astropy.git (push)
upstream	git://github.com/astropy/astropy.git (fetch)
upstream	git://github.com/astropy/astropy.git (push)

you mean delete the master branch altogether in the local checkout (and origin, by inference)? How would I go about testing the current master then, or fork the next branch from it?
The thing I may have missed was switching to the topical branch first, as git help rebase suggested that

git rebase master python-38-ssize_t+int-fix

was exactly equivalent (even implicitly checking out the topical branch)... And I fetched upstream, possibly not origin, since I had last updated the remote branch from this exact repo. But at that point git already began refusing to push upstream (correct: push, meaning push origin), demanding that I first pull again from the remote and so on and so on, and I wanted to avoid using the --force as long as possible.
Will try the interactive mode next time, hopefully that willl give me a warning before I push something online.

[dhomeier]

Mmh, I also did not wish to rewrite history, but this seemed the best way out now, as I did not want to decide on merging others' commits either. Still don't quite get the stupid warning about

hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.

as there are no remote changes I wish to add locally (OK, I do see that the remote tip was 912ed85, which no longer exists in the merged branch, but I still don't think I'd want to pull it to reapply it locally)... Anyway being forced to --force looks like an ugly way out, but at least appeared to work; thanks @bsipocz!

[bsipocz]

you mean delete the master branch altogether in the local checkout (and origin, by inference)? How would I go about testing the current master then, or fork the next branch from it?

the thing that we you did that from your origin/master it wasn't the actual top of the master branch.
Instead now you can do git checkout upstream/master. Or even create a trailing/staging/placeholder/etc branch that is effectively your current origin/master without the name confusion.

[bsipocz]

git already began refusing to push upstream

you can't push to upstream only to your fork. This possible confusion about the names, origin vs upstream is the reason we suggest to rename them to upstream --> astropy, origin-->your_github so it's super clear where those live on github.

[bsipocz]

and I wanted to avoid using the --force as long as possible.

A rebase rewrites history, so in almost all cases you need to use --force

[dhomeier]

Yep, the upstream was incorrect, but only in my description - I actually did a push without arguments the first time, git push origin python-38-ssize_t+int-fix the second time, and got the same rejection notice both times around. And I did fetch and rebase upstream/master my local master immediately before first trying to rebase the branch, so from my understanding as well as checking of the logs both master branches should have been identical. I suspect letting myself getting intimidated to pull the remote origin into python-38-ssize_t+int-fix again was the actual mistake...
Anyway, will be more explicit in which repository I am merging in the future. Needing a --force in a basically standard work flow takes getting used to.

[bsipocz]

So now as I see the base of the branch is still on your fork's master. Repeating the rebase (now on top of upstream/master) should automatically identify your 2 commits and will but it top of the current astropy master. Never mind me. The PR is OK now, I just didn't fetch what you've done and looked at an obsolete version of this locally.

[bsipocz]

Thanks @dhomeier!

[dhomeier]

Thanks @bsipocz, never mind my ranting, I was just keeping notes of what I found odd or confusing in the work pattern (and where I would have git expected to be smarter)!

[bsipocz]

@dhomeier - PRs are welcome to our dev documentation. It's actually helpful if when people are pointing out which part was confusing or not helpful enough.