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

Importing ABCs through compatibility layer and fix of proj_version check #2442

Merged
merged 10 commits into from Sep 13, 2019

Conversation

@heavelock
Copy link
Contributor

commented Aug 28, 2019

Okay, this time I hope it is done properly...

I replaced bare imports of Abstract Base Classes to go through compatibility layer. It is necessary for compatibility between 2.7 and 3.8 since 3.8 drops possibility of ABC import directly from collections and requires import from collections.abc.
I substituted imports only for ABCs so Mapping, Iterable and Sequence. There are only so few changes because in other cases the imports were done already in that way.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@heavelock heavelock requested review from QuLogic and megies Aug 28, 2019

obspy/clients/fdsn/mass_downloader/restrictions.py Outdated Show resolved Hide resolved
obspy/signal/quality_control.py Outdated Show resolved Hide resolved
@d-chambers

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Hey @heavelock,

Thanks for fixing this. These warnings were also starting to annoy me but not quite enough to do a PR myself 😉

Can you please change lines 385-388 (ish) in obspy.core.util.base to this:

    _proj = Proj(proj='utm', zone=10, ellps='WGS84')
    version_string = str(getattr(_proj, 'proj_version_str', 'proj_version'))
    if raw_string:
        return version_string

or something similar to fix Travis failures on python 3.7? Since this is a compatibility type PR I think it belongs here. I don't think it is that important to address all the 2.7 failures before merge, but we should at least keep make sure 3.7 runs.

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Yup, here it comes. On my 3.7 runs, let's see the CI.

@megies

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Python 3.8 has b4 out, btw, might be worthwile checking that as well

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

As far as I can see guys at coda-forge are still working on preparing py3.8 package so we have to wait for them. conda-forge/python-feedstock#271

obspy/core/util/base.py Outdated Show resolved Hide resolved
@megies
Copy link
Member

left a comment

see comment, i think needs a change

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Hmm, Travis has pyproj 2.3.1 which seems not to have attribute proj_version in the Proj class. it has however pyproj.proj_version_str. Here it's being used directly http://pyproj4.github.io/pyproj/stable/_modules/pyproj/_show_versions.html#show_versions
Let's see.

@heavelock heavelock changed the title Importing ABCs through compatibility layer Importing ABCs through compatibility layer and fix of proj_version check Sep 3, 2019

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I removed a check for proj_version_str attribute because it was pointless - that version of attribute was added i think somewhere >2.0 so it would not work anyway. In 2.3.1 import directly from pyproj works.

I think it's ready to merge although the py3.7 still doesn't pass due to FileNotFoundError: [Errno 2] No such file or directory: '/home/travis/miniconda/envs/test-environment/share/proj/epsg'.

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

In order to solve those fails at py37 we would need to pin basemap to 1.2.1. In that versionn they copied data to their own repo:
matplotlib/basemap@84ddf45
Should I do it?

@d-chambers

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Oops, my bad sorry for misleading you @heavelock.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

In order to solve those fails at py37 we would need to pin basemap to 1.2.1

I think this is a sensible thing to do. Dropping basemap is on the python 3 road map so it would be only for obspy 1.2 anyway. Do you see any issue @megies?

@megies

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

I think this is a sensible thing to do. Dropping basemap is on the python 3 road map so it would be only for obspy 1.2 anyway. Do you see any issue @megies?

I don't care too much, thats packaging problems of other packages so whatever works for our CI I guess

@megies
megies approved these changes Sep 4, 2019
Copy link
Member

left a comment

Looks good to me

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

I also added E/W504 to be ignored in circleci since we talked about that in #2444

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Okay, so it turnes out there is a conflict in requirements... Any clues how should I solve it?

Package proj4 conflicts for:

basemap=1.2.1 -> proj4[version='>=4.9.3,<6|>=5.2.0,<5.2.1.0a0']

basemap=1.2.1 -> pyproj[version='>=1.9.3'] -> proj4[version='>=5.2,<6']

proj4=4.9.3

pyproj -> proj4[version='6.0.0.*|>=4.9.3,<4.9.4.0a0|>=5.0.1,<5.0.2.0a0|>=5.1.0,<5.1.1.0a0|>=5.2,<6|>=5.2.0,<5.2.1.0a0|>=6.0.0,<6.0.1.0a0|>=6.1.0,<6.1.1.0a0|>=6.1.1,<6.1.2.0a0']
@megies

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Why do we have to pin proj to a specific version? Can we not allow a certain range of versions? Also the basemap issue 433 mentioned in the pinning comment is about a pip install while in Travis we use conda? I'm a bit confused.

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Wasn't it also connected to flipping east-west in some combinations of proj4 and basemap?
I can just release those constraints for 3.7 and see if the mole people maps come back on Travis.

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

It looks it worked. The problem is that a new test failure occurred, from io module. I will do rebase and see what is Travis's answer.

PS: @megies thanks for finalizing so many PRs today!

@heavelock heavelock force-pushed the heavelock:origin/fixwarnings branch from 5b8c9e1 to 2fdb4df Sep 12, 2019

@megies

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

finalizing so many PRs today!

trying to weed out https://github.com/obspy/obspy/projects/2 we need to push

@heavelock

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Okay, it looks like the PR is ready to go. Fails look to be unrelated. I will rebase once again and it will be ready to merge whenever you ready.

@heavelock heavelock force-pushed the heavelock:origin/fixwarnings branch from 35bffe4 to b58b00b Sep 12, 2019

@heavelock heavelock referenced this pull request Sep 12, 2019
0 of 20 tasks complete
@megies

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Lookin good, thanks a lot @heavelock 🎉

@megies megies added the PY2/3 label Sep 13, 2019

@megies megies added this to the 1.2.0 milestone Sep 13, 2019

@megies megies merged commit 0282a5a into obspy:master Sep 13, 2019

0 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
docker-testbot docker testbot results not available yet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.