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

py3k deps update #220

Closed
wants to merge 9 commits into from
Closed

Conversation

tomkralidis
Copy link
Member

No description provided.

@tomkralidis tomkralidis requested a review from justb4 October 12, 2018 10:39
Copy link
Member

@justb4 justb4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention to have Python3 compat? Great! One point though: we usually do big PRs on the dev branch which updates dev.geohealthcheck.org (though I am also not always consistent) and when stable merge dev into master. But let's aim for master here. Saw one failing check: https://geo.woudc.org/csw but in other GHC instance is ok, could be temporary?

Changes overall look good, one main point: you should update .travis.yml for Python3 tests (if Py3 is the intention).

@tomkralidis
Copy link
Member Author

@justb4 the changes are to safeguard and work for both Python 2 and 3. Having said this, moving to Python 3 as the default is a good idea. I'll try that, as well as fix some test problems.

Copy link
Member

@justb4 justb4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Py2 can still be supported. Many deployments, including Docker still use 2.7. Also it is common to support multiple Py3 versions, list would be:

  - 2.7
  - 3.4
  - 3.5
  - 3.6
  - 3.7 (?)

@justb4
Copy link
Member

justb4 commented Oct 12, 2018

Hmm, I begin to realize that moving to Python3 and supporting also 2.7 can have quite a few issues. For example: https://stackoverflow.com/questions/11914472/stringio-in-python3 but also the Probe Plugin system does quite some intricate reflection stuff (i.e. creating Objects from strings), that may not work in Py3.

Also (see our personal Gitter conversation, you may have missed my msg from sept 18) I would rather first bring out 0.4.0 version as the master branch is now stable for some time. See my proposal in Gitter. So I propose this order:

  • bring out v0.4.0
  • move the test servers to uptodate Ubuntu/python servers using Docker deployment
  • merge in the branch "per resource scheduling" I have been working on since the code sprint and is used in various deployments by me
  • support Python3 via the dev branch

Sorry, my initial reaction was "go" but then I realized the other stuff waiting and implications.

@tomkralidis
Copy link
Member Author

@justb4 +1/agree, let's get 0.4.0 out of the way as the first step.

@justb4
Copy link
Member

justb4 commented Nov 4, 2018

@tomkralidis Yes 0.4.0 first. Then I propose to merge in this branch to master: https://github.com/geopython/GeoHealthCheck/tree/per_resource_scheduling .
Kept that branch even with master.
And then outstanding fixes+py3. I have more time to work on GHC coming 2 months.

@justb4
Copy link
Member

justb4 commented Mar 19, 2019

We need to do this at some point. But like many projects as Django (2.0 is PY3 only) etc. I would prefer not to have simultaneous Py2 and Py3 support, given that:

  • Py2 will be EOL very soon (less than a year)
  • maintenance: we already have limited human resources
  • many users (more and more) deploying with Docker

Like with Stetl in a similar PR we decided to:

  • bring out one last Py2 release (and Docker Image)
  • make branch of that release, for urgent fixes only
  • bring out fresh Py3-only version

@tomkralidis
Copy link
Member Author

Closing in lieu of #280

@tomkralidis tomkralidis deleted the py3k-deps-update branch November 14, 2019 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants