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

Set python3 as default interpreter #3581

Merged
merged 12 commits into from Feb 6, 2019
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 6, 2018

This closes #3070

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 6, 2018

Do I need to run migrations for this? If that is the case, probably we want to do another PR with the missing migrations right now before this one.

@@ -196,7 +196,7 @@ class Project(models.Model):
_('Python Interpreter'),
max_length=20,
choices=constants.PYTHON_CHOICES,
default='python',
default='python3',
help_text=_('(Beta) The Python interpreter used to create the virtual '
Copy link
Member Author

@stsewd stsewd Feb 6, 2018

Can I remove the help text about being a beta feature?

Copy link
Contributor

@agjohnson agjohnson Feb 13, 2018

Oh yeah, this copy should definitely change :)

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 9, 2018

This is a change we've been wanting to make for a while now, but it is blocked more on the operations and support side. If we don't want to jump into this change just yet, we can at least keep this PR sidelined for when we are close.

I think we want to eventually just force 3.6 (at least). So perhaps the order of how we merge things would look like:

  • Make our latest docker image the new 3.0 and make that the default. This image has 3.6
  • Update this PR to make 3.6 the default python interpreter
  • Merge and deploy this

There is also likely a change we should coordinate here with our configuration handling and we should make 3.6 the default there too.

@humitos
Copy link
Member

@humitos humitos commented Mar 23, 2018

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 8, 2018

Not blocked on readthedocs/readthedocs-docker-images#62, but this is blocked on making a change to our default image.

@agjohnson agjohnson added this to the 2.7 milestone Jun 8, 2018
@stsewd stsewd force-pushed the python3-default-env branch from a72d1f2 to a4406db Jun 14, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jun 19, 2018

So, this will change the default interpreter for new projects, which is probably a safe place to start. We probably need to define a few more things though:

  • This will make the default database field 'python3', but have we tested what projects will get by default with the config parsing step? We do some weird things in both the rtd config parsing and the rtd-build config parsing where we rely on the default of the respective field value to assume the user hasn't defined anything special. Do we need to change anything here?
  • Do we need a default null value in this field instead?
  • Do we want to leave existing projects alone? I don't think there is any way we can determine whether a project is purposely selecting python as the interpreter or if it was just the default.
  • It sounds like we're waiting until at least the docker 3.0 image is selectable, we could probably target 3.6 instead of 3.5

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 26, 2018

Do we want to leave existing projects alone? I don't think there is any way we can determine whether a project is purposely selecting python as the interpreter or if it was just the default.

Yes. This should only be for projects going forward, not a change of existing configurations.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 22, 2018

With the config file v2, the default version is python3, v1 would respect the setting in the admin panel. This only affects new projects.

@stsewd stsewd force-pushed the python3-default-env branch from a4406db to 29e4fa0 Aug 22, 2018
@@ -135,7 +135,7 @@ used for building documentation.
python.version
``````````````

* Default: ``2.7``
* Default: ``3.5``
Copy link
Member Author

@stsewd stsewd Aug 22, 2018

This will be difficult to express... The actual default value here is the one from the admin panel

Copy link
Member

@humitos humitos Jan 2, 2019

3.5 has some issues (#5051) we may want to default to 3.6 or even 3.7.x because

Python 3.6.8 is planned to be the last bugfix release for 3.6.x. Following the release of 3.6.8, we plan to provide security fixes for Python 3.6 as needed through 2021, five years following its initial release.

from https://www.python.org/downloads/release/python-368/

This is also a good excuse to upgrade our Docker images.

Copy link
Member

@humitos humitos Jan 15, 2019

With our current release of docker images:

  • 3.0 will default to py3.5
  • 4.0 to py3.5
  • 5.0rc1 to py3.6

So, I think we are fine here.

@stsewd stsewd force-pushed the python3-default-env branch from 03723df to 4f4fd67 Oct 24, 2018
@codecov
Copy link

@codecov codecov bot commented Oct 24, 2018

Codecov Report

Merging #3581 into master will decrease coverage by 0.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3581      +/-   ##
==========================================
- Coverage   76.74%   76.26%   -0.49%     
==========================================
  Files         158      158              
  Lines        9951    10034      +83     
  Branches     1244     1267      +23     
==========================================
+ Hits         7637     7652      +15     
- Misses       1980     2039      +59     
- Partials      334      343       +9
Impacted Files Coverage Δ
readthedocs/projects/models.py 84.07% <ø> (-1.49%) ⬇️
readthedocs/builds/views.py 85.71% <0%> (-10.29%) ⬇️
readthedocs/builds/forms.py 87.87% <0%> (-7.96%) ⬇️
readthedocs/restapi/client.py 82.35% <0%> (-5.53%) ⬇️
readthedocs/vcs_support/backends/hg.py 59.09% <0%> (-5.43%) ⬇️
readthedocs/projects/utils.py 54.54% <0%> (-4.55%) ⬇️
readthedocs/restapi/views/integrations.py 87.23% <0%> (-4.54%) ⬇️
readthedocs/builds/models.py 75.98% <0%> (-2.85%) ⬇️
readthedocs/vcs_support/base.py 88.67% <0%> (-1.89%) ⬇️
readthedocs/vcs_support/backends/svn.py 27.69% <0%> (-1.82%) ⬇️
... and 38 more

@agjohnson agjohnson removed this from the 2.10 milestone Nov 10, 2018
@agjohnson agjohnson added this to the 3.0 milestone Nov 10, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 17, 2018

Sphinx 2.0 is deprecating python2 http://www.sphinx-doc.org/en/master/changes.html#incompatible-changes. Just a note in case we update sphinx first.

Copy link
Member

@humitos humitos left a comment

I want this to go out sooner than later.

This is a safe change considering that it will only affect new projects and won't touch existing ones.

Although, I'd prefer to upgrade our docker images before this PR gets merged and use 3.6 or probably 3.7 as default instead of 3.5.

@@ -135,7 +135,7 @@ used for building documentation.
python.version
``````````````

* Default: ``2.7``
* Default: ``3.5``
Copy link
Member

@humitos humitos Jan 2, 2019

3.5 has some issues (#5051) we may want to default to 3.6 or even 3.7.x because

Python 3.6.8 is planned to be the last bugfix release for 3.6.x. Following the release of 3.6.8, we plan to provide security fixes for Python 3.6 as needed through 2021, five years following its initial release.

from https://www.python.org/downloads/release/python-368/

This is also a good excuse to upgrade our Docker images.

@humitos
Copy link
Member

@humitos humitos commented Jan 2, 2019

Although, I'd prefer to upgrade our docker images before this PR gets merged and use 3.6 or probably 3.7 as default instead of 3.5.

I'm proposing some direction to achieve this goal at readthedocs/readthedocs-docker-images#84 but it involves some months before we get 3.7 working on the docker image (we have to release 4.0 first).

So, I suppose we can do this a smooth migration: first go with 3.6 and then with 3.7. Also, since we are setting only 3 as Python default version here, it will use the default python3 for the image itself. So, to upgrade to 3.7 we will only need to change the pyenv global (which currently is 3.6 https://github.com/rtfd/readthedocs-docker-images/blob/releases/3.x/Dockerfile#L84-L89 but it will be 3.7 after merging my PR)

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jan 2, 2019

Sphinx 2.0 is deprecating python2

Sphinx2 is doing a lot of things and I suspect there will be a number of incompatibilities (with our theme possibly but also lots of folks' docs). We will probably have to test this pretty extensively and perhaps have a milestone for it.

@@ -135,7 +135,7 @@ used for building documentation.
python.version
``````````````

* Default: ``2.7``
* Default: ``3.5``
* Options: ``2.7``, ``2``, ``3.5``, ``3``
Copy link
Member

@humitos humitos Jan 15, 2019

I suppose that we should update this line to support 3.6 also.

@humitos
Copy link
Member

@humitos humitos commented Jan 15, 2019

I think we are almost ready to merge this PR.

  • new docker images shipped
  • default python in documentation is accurate
  • update docs for possible values with 3.6 (#4451)
  • update python code (YAML schema and Config object) to accept 3.6 as a value (#5155)

Am I missing anything else here?

@agjohnson agjohnson removed this from the 3.0 milestone Jan 25, 2019
@agjohnson agjohnson added this to the 3.2 milestone Jan 25, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Everything looks good here 👍

Copy link
Member

@humitos humitos left a comment

I think that after updating the docs about the possible values for build.image we are ready to merge.

What I'm reading in the code does not match what we have written in the docs, but maybe I'm wrong.

docs/config-file/v1.rst Show resolved Hide resolved
docs/config-file/v1.rst Outdated Show resolved Hide resolved
@stsewd stsewd requested a review from Feb 6, 2019
humitos
humitos approved these changes Feb 6, 2019
@stsewd stsewd merged commit 5e2c781 into readthedocs:master Feb 6, 2019
1 of 2 checks passed
@stsewd stsewd deleted the python3-default-env branch Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants