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

Add default_python_version to DOCKER_IMAGE_SETTINGS #6551

Closed
humitos opened this issue Jan 21, 2020 · 11 comments
Closed

Add default_python_version to DOCKER_IMAGE_SETTINGS #6551

humitos opened this issue Jan 21, 2020 · 11 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Sprintable Small enough to sprint on

Comments

@humitos
Copy link
Member

humitos commented Jan 21, 2020

We are currently configuring available docker images for builders at

DOCKER_IMAGE_SETTINGS = {
'readthedocs/build:1.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.4]},
},
'readthedocs/build:2.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5]},
},
'readthedocs/build:3.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.3, 3.4, 3.5, 3.6]},
},
'readthedocs/build:4.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7]},
},
'readthedocs/build:5.0': {
'python': {'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7, 'pypy3.5']},
},
'readthedocs/build:6.0rc1': {
'python': {'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7, 3.8, 'pypy3.5']},
},
}

At the moment, we don't have a way to pin which version of Python will be used by default on each of these images. This code says that the default Python version will be always the greatest on each image:

@property
def python_interpreter(self):
ver = self.python_full_version
if not ver or isinstance(ver, (int, float)):
return 'python{}'.format(ver)
# Allow to specify ``pypy3.5`` as Python interpreter
return ver
@property
def python_full_version(self):
ver = self.python.version
if ver in [2, 3]:
# Get the highest version of the major series version if user only
# gave us a version of '2', or '3'
ver = max(
v for v in self.get_valid_python_versions()
if not isinstance(v, str) and v < ver + 1
)
return ver

We want to update the DOCKER_IMAGE_SETTINGS to have a default_python_version per image and those linked properties to use them if defined.

This allows us to release a new version of Docker image with a newer version of Python without forcing all users using latest to automatically start using the newer version of Python.

Required by: #6324

@humitos humitos added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Jan 21, 2020
@saadmk11
Copy link
Member

saadmk11 commented Jan 21, 2020

@humitos It seems in python_full_version property we are tying to get the max for each major versions of python (2, 3). So, If we add only one default version then how are we going to differentiate between python 2 and 3. for example, if user sets 2 as the version and we have 3.6 as default.

@stsewd
Copy link
Member

stsewd commented Jan 21, 2020

@saadmk11 yeah, we should implement it like

'default_version': {
    2: 2.7,
    3: 3.7,
} 

@preetmishra
Copy link
Contributor

@stsewd Will adding a default_version key next to supported_versions key in every build do the work? Also, which python version should be the default in every build? (the oldest one?)

@joaovitor3
Copy link

Anyone is working in this issue?
I have added default version using the latest python version listed in supported versions, as @preetmishra mentioned:

'readthedocs/build:6.0rc1': {
    'python': {
        'supported_versions': [2, 2.7, 3, 3.5, 3.6, 3.7, 3.8, 'pypy3.5'],
        'default_version': {
            2: 2.7,
            3: 3.8,
        }
    },
},

This solve this issue completely?

@preetmishra
Copy link
Contributor

@joaovitor3 Yes, I'd like to work on this issue, if it seems doable. I am waiting for the reply from @stsewd.

@joaovitor3
Copy link

No problem, you can work on this @preetmishra
Thanks for the reply.

@humitos humitos added the Sprintable Small enough to sprint on label Jan 25, 2020
@humitos
Copy link
Member Author

humitos commented Jan 28, 2020

Will adding a default_version key next to supported_versions key in every build do the work?

No. You need to also find where this needs to be used (I linked the code in the description). Please, add some tests for this and make sure that it picks the right Python version when building in your RTD instance.

Also, which python version should be the default in every build? (the oldest one?)

See the comment from @stsewd at #6551 (comment)

@preetmishra
Copy link
Contributor

@humitos Thanks for reverting back. I understood how default_version should be like from the comment.
I have a few more questions.

  • Should the default python version be the same for every build or should it be different?
  • Which python version should we choose? The oldest version (say 2 for python 2 and 3 for python 3 for readthedocs/build:6.0rc1) or the newest version (say 2.7 for python 2 and 3.8 for python 3 for readthedocs/build:6.0rc1)?

@Paebbels
Copy link

Paebbels commented Feb 4, 2020

@humitos sorry, but you complete container build approach is wrong...

At first, containers include software and libraries they need to fulfill their purpose. Having all Python versions in parallel in on container is wrong.

At second, (Docker) containers are build in layers. You create a base container and on top you create variations. That's like a Git commit on branch master and having multiple branches diverting from from that base commit.

  1. Build base containers for an environment supporting Pythons 2.7 and 3.3 ... 3.8
  2. Use a Python x.y container to create derived containers for RTD 1.0 ... 6.0rc1

This should result in these containers:

  • readthedocs/build-py2.7:1.0
  • readthedocs/build-py2.7:2.0
  • readthedocs/build-py2.7:3.0
  • readthedocs/build-py2.7:4.0
  • readthedocs/build-py2.7:5.0
  • readthedocs/build-py2.7:6.0rc1
  • readthedocs/build-py3.3:3.0
  • readthedocs/build-py3.4:1.0
  • readthedocs/build-py3.4:3.0
  • readthedocs/build-py3.5:2.0
  • readthedocs/build-py3.5:3.0
  • readthedocs/build-py3.5:4.0
  • readthedocs/build-py3.5:5.0
  • readthedocs/build-py3.5:6.0rc1
  • readthedocs/build-py3.6:3.0
  • readthedocs/build-py3.6:4.0
  • readthedocs/build-py3.6:5.0
  • readthedocs/build-py3.6:6.0rc1
  • readthedocs/build-py3.7:4.0
  • readthedocs/build-py3.7:5.0
  • readthedocs/build-py3.7:6.0rc1
  • readthedocs/build-py3.8:6.0rc1

If you think about resource consumption, that not a big deal, because all build-pyX.Y versions share the same container layer. Only your application on top creates overhead, because you do a matrix build. You could now review if you need a Cartesian product or if a sparse matrix is enough.

You don't run into a multi-version and default-version problem, because your image selected the appropriate Python environment.

@humitos
Copy link
Member Author

humitos commented Feb 5, 2020

Thanks for the tip, @Paebbels. There are other things to consider as well before changing how we handle our docker images. However, I think the effort we need for adding a config setting (current issue) is very low compared to changing all of this, and we will get the same benefit.

@humitos
Copy link
Member Author

humitos commented Feb 19, 2020

Done at #6653

@humitos humitos closed this as completed Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Sprintable Small enough to sprint on
Projects
None yet
Development

No branches or pull requests

6 participants