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

Sunset py2 #2577

Open
wants to merge 34 commits into
base: master
from
Open

Sunset py2 #2577

wants to merge 34 commits into from

Conversation

@megies
Copy link
Member

megies commented Mar 9, 2020

What does this PR do?

WIP Removes all Python 2 compatibility from the code base

Why was it initiated? Any relevant Issues?

Next major release after 1.2.0 will be Python 3 only.

No PRs are to be merged into master before this one

Supersedes #1613

Fixes #2228

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 .
@megies megies added this to the 2.0.0 milestone Mar 9, 2020
@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 9, 2020

WIP just drop a note here if you (intend to) work on this.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 9, 2020

ping I can do stuff if needed, just let me know.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 9, 2020

the CI issues seems to be linked to this: pypa/setuptools#2017 with setuptools 46.0

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 9, 2020

Yeah this: pypa/setuptools#65

I'm gonna stop here for a while. Most of the brute force regex patching is done, after adapting to that setuptools change it will likely be looking at failing tests in detail.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 9, 2020

@QuLogic I have a feeling you might use this mechanics to switch off libmseed for your packaging that has it packaged externally? So maybe you have some input on how to adapt to that setuptools change, perhaps?

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 10, 2020

image
that won't help :-)

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 10, 2020

you could try to debug it with python -m pdb /path/to/obspy-runtests maybe

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 11, 2020

you could try to debug it with python -m pdb /path/to/obspy-runtests maybe

it's not on my machine, it's on travisCI :-)

@megies megies force-pushed the sunset_py2 branch from bd78088 to c409d56 Mar 11, 2020
@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 11, 2020

Force pushed after getting the setuptools fix into master for 1.2.1

@megies megies mentioned this pull request Mar 11, 2020
8 of 17 tasks complete
setup.py Outdated Show resolved Hide resolved
megies added 13 commits Mar 9, 2020
@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Mar 19, 2020

I made some progress. I need to work on a paper now but I should be able to come back to this next week. If anyone else wants to pick it up feel free.

There are a slew of network issues and potentially some matplotlib issues. I bumped the required MPL version to 3.2 as suggested by @heavelock here and got a dozen or so failures locally but travis seems to have handled it better. We should probably take a closer look at the implications of upgrading matplotlib.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 20, 2020

We should probably deactivate all arclink tests and instead show a Deprecation Warning when arclink is used. The test server at GFZ is shut down and while we could switch to a different server for now, I don't think it's worth it.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 20, 2020

We should recreate all matplotlib test images on 3.2, maybe i'll get to it at some point

@megies megies force-pushed the sunset_py2 branch from ff5f7f2 to b0ed739 Mar 20, 2020
@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 25, 2020

Good shout @d-chambers. Since this seems to be depending on external C implementations and not strictly depending on Python version or implementation, maybe an introspective approach might be safer. I've made a change, please have a look and let me know if you see a problem with it.

@megies megies force-pushed the sunset_py2 branch from 4d44902 to 9bfa573 Mar 25, 2020
@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Mar 25, 2020

@megies,
The code is not particularly aesthetic, and I am not a huge fan of setting class attribute like that, but it certainly is a more comprehensive solution to a somewhat obscure edge case. A better way of doing it isn't coming to mind, so let's roll with it 😉

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 26, 2020

From the current test reports on network tests from Travis, to me it looks like there are no problems coming from dropping py2/3 compatibility stuff and future: http://tests.obspy.org/?pr=2577

@ThomasLecocq can you run network tests on Windows for this one and send to report server? Also if somebody with Mac can run network tests for this one, that'd be awesome (@krischer?).

For obspy-runtests use ..

  • -a to run all tests including network tests
  • --pr-url="https://github.com/obspy/obspy/pull/2577" to make it easier to find the test when looking for it (it can be looked up by PR number then)
@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 26, 2020

running in an existing env (that has future etc)... will create a fresh empty one to test afterwards

@aringler-usgs

This comment has been minimized.

Copy link
Member

aringler-usgs commented Mar 26, 2020

I just ran it on a mac. Let me know if you want this run again.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 27, 2020

Thanks @ThomasLecocq and @aringler-usgs. Looks like we have no systematic issues stemming from the Py2 compatibility layer drop. 🚀

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 27, 2020

@megies should I test in a fresh 3.7 ot 3.8 env ? do you have a conda create recipe?

@megies megies mentioned this pull request Mar 27, 2020
8 of 9 tasks complete
@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 27, 2020

@megies should I test in a fresh 3.7 ot 3.8 env ? do you have a conda create recipe?

I think you can just install 1.2.0 via packages in a fresh env and remove obspy and future, then install this branch with pip.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 27, 2020

I think this can be merged, but can never hurt for somebody else to give it another look. Other action items in #2228 seem secondary and can be worked on afterwards, I think, without conflicting with new PRs then too much.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 27, 2020

@megies should I test in a fresh 3.7 ot 3.8 env ? do you have a conda create recipe?

I think you can just install 1.2.0 via packages in a fresh env and remove obspy and future, then install this branch with pip.

so I did conda create -n TMP -c conda-forge python=3.7 obspy==1.2.0 --only-deps and removed future

launching tests...

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 27, 2020

done

@ThomasLecocq ThomasLecocq mentioned this pull request Mar 30, 2020
6 of 6 tasks complete
@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Mar 30, 2020

is there an easy way to get rid if those mopad warnings ? (I mean obviously rewriting the code is fine, but dunno if it's really needed?)

/home/travis/miniconda/envs/test-environment/lib/python3.7/site-packages/obspy/imaging/scripts/mopad.py:213: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
  M[4], M[5], M[2]]).reshape(3, 3)
@megies

This comment has been minimized.

Copy link
Member Author

megies commented Mar 31, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release 2.0.0
In Progress
Release 2.0.0
In Progress
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.