Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

Onboard python-3 to image build #241

Merged
merged 11 commits into from
Mar 27, 2020
Merged

Onboard python-3 to image build #241

merged 11 commits into from
Mar 27, 2020

Conversation

Chuxel
Copy link
Member

@Chuxel Chuxel commented Feb 21, 2020

This is a continuation of PR #238 which originally included on-boarding Python into the image build process for #154.

To recap, this PR covers on-boarding Python images into vscode/devcontainers/python repository in MCR. Since Python's currently in support versions are 3.5 through 3.8, this PR generates separate images per version from the same python-3 definition.

So you get:

  • vscode/devcontainers/python:3
  • vscode/devcontainers/python:3.8
  • vscode/devcontainers/python:3.7
  • vscode/devcontainers/python:3.6
  • vscode/devcontainers/python:3.5

...along with a "latest" that ties to python:3 and then version specific tags (e.g. python:0.102.0-3.6)

Discussion: The discussion in #238 is whether we would be better off having a single "python" image that includes all supported versions rather than separate images per version.

Pro: The upside is that developers can just grab a single image and then use virtual env to pick their version.
Con: We would end up with a larger image and people practically speaking would be forced into using virtual env. We would also need to implement separate image versioning for the python image so that we could increment the major version number any time we removed a python version since this would be breaking. That makes finding the version number harder since it wouldn't mirror the VS Code extension anymore.

The question is which approach is best for Python.

//cc: @chrmarti @brettcannon @egamma

@brettcannon
Copy link
Member

If you're developing an app you're only going to care about a single Python version, but if you're developing a library you're going to want a bunch of versions.

I say do what makes your life easier which sounds like individual images. For those of us who need more than that we can edit the Dockerfile to pull in e.g. deadsnakes if the base image is Ubuntu (in which case maybe include a comment about this?).

@Chuxel
Copy link
Member Author

Chuxel commented Mar 9, 2020

Apologies for the delay here - had some unexpected OOF'ness last week. Makes sense. Adding the comment also is a good idea. I can tweak the PR.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Marking as "request changes" to get out of my review queue and as a reminder to add that comment.

@Chuxel
Copy link
Member Author

Chuxel commented Mar 18, 2020

I've adapted to the feedback. This will now generate separate images, but also includes instructions in base.Dockerfile for building one with multiple version of Python inside it.

In VS Code, the Dockerfile in this folder is used (though the image tag is updated) which includes some customizations not in the generic Debian Dockerfile (under build/assets/debian.Dockerfile).

The resulting output looks like this:

{
    "name": "Python 3",
    "context": "..",

    // Update the FROM statement in this Dockerfile to pick a version of Python.
    "dockerFile": "Dockerfile",

    // Set *default* container specific settings.json values on container create.
    "settings": {
        "terminal.integrated.shell.linux": "/bin/bash",
        "python.pythonPath": "/usr/local/bin/python",
        "python.linting.enabled": true,
        "python.linting.pylintEnabled": true,
        "python.linting.pylintPath": "/usr/local/bin/pylint"
    },

    // Add the IDs of extensions you want installed when the container is created.
    "extensions": [
        "ms-python.python"
    ]

    // Use 'forwardPorts' to make a list of ports inside the container available locally.
    // "forwardPorts": [],

    // Use 'postCreateCommand' to run commands after the container is created.
    // "postCreateCommand": "pip3 install -r requirements.txt",

    // Uncomment to connect as a non-root user. See https://aka.ms/vscode-remote/containers/non-root.
    // "remoteUser": "vscode"
}
# To fully customize the contents of this image, use the following Dockerfile instead:
# https://github.com/microsoft/vscode-dev-containers/tree/clantz/python-onboard/containers/python-3/.devcontainer/base.Dockerfile
FROM mcr.microsoft.com/vscode/devcontainers/python:0-3

# Additional Python images available (update FROM to use): 
# - mcr.microsoft.com/vscode/devcontainers/python:3.8
# - mcr.microsoft.com/vscode/devcontainers/python:3.7
# - mcr.microsoft.com/vscode/devcontainers/python:3.6
# - mcr.microsoft.com/vscode/devcontainers/python:3.5

# [Optional] If your requirements rarely change, uncomment this section to add them to the image.
#
# COPY requirements.txt /tmp/pip-tmp/
# RUN pip3 --disable-pip-version-check --no-cache-dir install -r /tmp/pip-tmp/requirements.txt \
#    && rm -rf /tmp/pip-tmp

# [Optional] Uncomment this section to install additional packages.
#
# ENV DEBIAN_FRONTEND=noninteractive
# RUN apt-get update \
#    && apt-get -y install --no-install-recommends <your-package-list-here> \
#    #
#    # Clean up
#    && apt-get autoremove -y \
#    && apt-get clean -y \
#    && rm -rf /var/lib/apt/lists/*
# ENV DEBIAN_FRONTEND=dialog

@Chuxel Chuxel requested a review from brettcannon March 19, 2020 14:14
@Chuxel Chuxel changed the title [WIP] Onboard python-3 to image build Onboard python-3 to image build Mar 19, 2020
@Chuxel
Copy link
Member Author

Chuxel commented Mar 19, 2020

@brettcannon Since pipx requires Python 3.6, I stuck with a virtualenv, but that update is in now too.

While we are at it, are there other utilities that should be included aside from pip, virtualenv, and pylint?

@Chuxel Chuxel requested a review from brettcannon March 19, 2020 23:04
@brettcannon
Copy link
Member

Since pipx requires Python 3.6, I stuck with a virtualenv

Or just don't ship a Python 3.5 image. 😁 I mean it reaches EOL in September so it isn't long for this world and hopefully your work here will outlast September.

The other issue is you are putting all the tools in a single virtual environment, which means they will potentially conflict with each other due to their dependencies (we have this problem in our testing of the extension as some tools require older versions of things that newer tools don't like).

are there other utilities that should be included aside from pip, virtualenv, and pylint?

It depends on how far you want to go with this. 😄 If you just want the popular ones, then in no particular order:

  1. pylint
  2. flake8
  3. autopep8
  4. black
  5. pytest
  6. virtualenv

If you want the complete list of tools that the Python extension supports, that adds in no particular order:

  1. yapf
  2. mypy
  3. pydocstyle
  4. pycodestyle
  5. prospector
  6. pylama
  7. bandit

@Chuxel
Copy link
Member Author

Chuxel commented Mar 20, 2020

Or just don't ship a Python 3.5 image. grin I mean it reaches EOL in September so it isn't long for this world and hopefully your work here will outlast September.

The other issue is you are putting all the tools in a single virtual environment, which means they will potentially conflict with each other due to their dependencies (we have this problem in our testing of the extension as some tools require older versions of things that newer tools don't like).

Ah, that last part is interesting in particular. Ok, sounds like we should just drop 3.5 then - I was trying to include "supported" versions, but 3.5 is pretty old. I was originally using pipx to install in this same location and then switched - I'll undo that and remove 3.5.

It depends on how far you want to go with this.

Honestly, it's a mainly an image-size vs benefit question. With the pre-built images, install time isn't as much a factor anymore which was the main reason to keep things tight before. It sounds like the first set would make sense at a minimum. Folks that were interested in the Go definition ended up submitting a PR to add everything the extension supported, so we could do that here as well if you think that makes sense.

@brettcannon
Copy link
Member

it's a mainly an image-size vs benefit question.

None of these tools will be large (they are all pure Python code), so it's probably fine to just go ahead and install all of them.

@Chuxel
Copy link
Member Author

Chuxel commented Mar 20, 2020

@brettcannon Okay, updated. Just installing pylint with pipx results in a 54M layer, with all the tools its 183M. The base image is pretty large to begin with however, so likely that's fine. See what you think.

@Chuxel Chuxel requested a review from brettcannon March 20, 2020 21:58
pylama \
bandit \
virtualenv \
pipx"
Copy link
Member

Choose a reason for hiding this comment

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

Should pipx be installed by pipx?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the question - would installing pipx globally affect dependencies too? I was instead installing in an alternate location temporarily and then isolating it so it was available for further use.

Copy link
Member

Choose a reason for hiding this comment

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

I guess if you're using pip to install pipx you're right that it should get installed on its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I looked at using apt-get, but this ended up requiring installation venv and other
OS dependencies as well all wired into the operating system version of python rather than the one from the image.

I could set VIRTUALENV_PYTHON to deal with the default there. Maybe that's enough? apt-get is certainly less gymnastics.

Copy link
Member

Choose a reason for hiding this comment

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

Getting tied to the system Python isn't the worst thing in the world. I think it's a 🤷‍♂️ decision at this point. 😉

containers/python-3/.devcontainer/base.Dockerfile Outdated Show resolved Hide resolved
containers/python-3/.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
@Chuxel Chuxel merged commit 2207ff0 into master Mar 27, 2020
@Chuxel Chuxel deleted the clantz/python-onboard branch April 6, 2020 13:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants