-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Dockerise testing for dev #1093
Changes from 4 commits
14ba6d6
b720398
aed52b7
27f02b9
3ddad59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
--- | ||
repos: | ||
- repo: https://github.com/psf/black | ||
rev: 22.12.0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
# Bakes the python versions which tsfresh targets into a testing env | ||
FROM ubuntu:22.04 | ||
|
||
SHELL ["/bin/bash", "-c"] | ||
|
||
# These are required to build python from source | ||
RUN apt-get update && apt-get install -y \ | ||
python3-pip \ | ||
curl \ | ||
clang \ | ||
git \ | ||
build-essential \ | ||
libssl-dev \ | ||
libreadline-dev \ | ||
zlib1g-dev \ | ||
libbz2-dev \ | ||
libsqlite3-dev \ | ||
llvm \ | ||
libncurses5-dev \ | ||
libgdbm-dev \ | ||
libnss3-dev \ | ||
libffi-dev \ | ||
liblzma-dev \ | ||
libgmp-dev \ | ||
libmpfr-dev \ | ||
&& apt-get clean | ||
|
||
|
||
RUN curl https://pyenv.run | bash | ||
|
||
# For interactive use (if any), this is an edge case. | ||
RUN echo 'export PYENV_ROOT="$HOME/.pyenv"' >> ~/.bashrc && \ | ||
echo '[[ -d $PYENV_ROOT/bin ]] && export PATH="$PYENV_ROOT/bin:$PATH"' >> ~/.bashrc && \ | ||
echo 'eval "$(pyenv init -)"' >> ~/.bashrc | ||
|
||
ENV PYENV_ROOT="/root/.pyenv" | ||
ENV PATH="$PYENV_ROOT/bin:$PATH" | ||
ENV PATH="$PYENV_ROOT/shims:$PATH" | ||
|
||
ARG PYTHON_VERSIONS | ||
RUN for version in $PYTHON_VERSIONS; do \ | ||
echo Installing $version; \ | ||
# Band aid for https://github.com/pyenv/pyenv/issues/1738 | ||
# since this also appears to apply to 3.7.X | ||
if [[ $version =~ ^3\.7\..*$ ]]; then \ | ||
echo Using clang to compile $version; \ | ||
CC=clang pyenv install $version || exit 1; \ | ||
else \ | ||
pyenv install $version || exit 1; \ | ||
fi; \ | ||
done | ||
|
||
RUN pyenv global $PYTHON_VERSIONS | ||
|
||
RUN pip install tox | ||
|
||
WORKDIR /tsfresh | ||
|
||
# Requires adding safe.directory so that tsfresh can build when the | ||
# repo is mounted. | ||
# Note cannot do this at build time as no git directory exists | ||
CMD ["/bin/bash", "-c", "git config --global --add safe.directory /tsfresh && tox -r -p auto"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
WORKDIR := /tsfresh | ||
TEST_IMAGE := tsfresh-test-image | ||
TEST_DOCKERFILE := Dockerfile.testing | ||
TEST_CONTAINER := tsfresh-test-container | ||
# >= 3.9.2 ---> https://github.com/dask/distributed/issues/7956 | ||
PYTHON_VERSIONS := "3.7.12 3.8.12 3.9.12 3.10.12 3.11.0" | ||
BLACK_VERSION := 22.12.0 | ||
# Isort 5.12.0 not supported with python 3.7.12 | ||
ISORT_VERSION := 5.12.0 | ||
|
||
build-testenv: | ||
docker build \ | ||
-f $(TEST_DOCKERFILE) \ | ||
-t $(TEST_IMAGE) \ | ||
--build-arg PYTHON_VERSIONS=$(PYTHON_VERSIONS) \ | ||
. | ||
|
||
# Tests `PYTHON_VERSIONS`, provided they are also | ||
# specified in setup.cfg `envlist` | ||
test-all-testenv: clean build-testenv | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More sensible make layout which cleans after the tests are invoked in docker: 3ddad59 Calls to However on reflection it makes more sense to move clean into executing after the docker tests run. |
||
docker run --rm \ | ||
--name $(TEST_CONTAINER) \ | ||
-v .:$(WORKDIR) \ | ||
nils-braun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
-v build_artifacts:$(WORKDIR)/build \ | ||
-v tox_artifacts:$(WORKDIR)/.tox \ | ||
nils-braun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
-v egg_artifacts:$(WORKDIR)/tsfresh.egg-info \ | ||
$(TEST_IMAGE) | ||
|
||
# Tests the python binaries installed | ||
# on local machine, provided they are also | ||
# specified in setup.cfg `envlist` | ||
test-all-local: clean | ||
tox -r -p auto | ||
|
||
# Tests for python version on local machine in | ||
# current context (e.g. global or local version of | ||
# python set by pyenv, or the python version in | ||
# the active virtualenv). | ||
test-local: clean | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question: why do you need to clean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to cleaning within the docker testing target 3ddad59 |
||
pip install .[testing] | ||
pytest | ||
|
||
clean: | ||
rm -rf .tox build/ dist/ *.egg-info | ||
|
||
install-formatters: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this command needed and the one below? We have pre-commit for exactly this, or? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed these targets 3ddad59 I was not aware that the precommit check runs locally with each commit, but I see that it is in the docs so we do not need this as you pointed out. |
||
pip install black==$(BLACK_VERSION) isort==$(ISORT_VERSION) | ||
|
||
format: install-formatters | ||
black --extend-exclude \.docs . | ||
isort --profile black --skip-glob="docs" . | ||
|
||
.PHONY: clean test-all-local test-local test-all-testenv format install-formatters |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,21 +64,32 @@ you have to: | |
|
||
.. code:: | ||
|
||
pytest | ||
make test-local | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this, users will need to reinstall the pip packages for testing on very run of the test. I know, it should be quick as all packages are already there, but it does take some extra time nevertheless. I would suggest to not add a makefile command for the local testing but to state that the users need to run pytest. The makefile command just replaced two commands and the installation command is explained on the page already anyways There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed all the one-line makefile commands 3ddad59 |
||
|
||
|
||
To test changes across multiple versions of Python, run: | ||
|
||
|
||
.. code:: | ||
|
||
tox -r | ||
make test-all-local | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above: I am not a big fan of replacing Single-Line commands with a makefile command. This adds another layer of indirection "advanced" users can be annoyed about (they know what tox is, so they want to potentially change options etc) and "new" users copy-paste what is in here anyways, so it does not matter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted the single line makefile commands + updated the docs 3ddad59 That makes sense. I was thinking that makefile is a useful place to act as a single point of entry for people new to the project to understand how to run it. But since the docs fulfill that purpose, understandable why that is not desired. Will remove the syntactic sugar. One advantage is that by encapsulating in a makefile, suppose tox -r evolves to tox -r -other_flag -another_flag etc... then using a makefile means the user only needs to think about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not know for tox, but many tools allow to set default options on the pyproject or setup.cfg or their own config file. This is typically better, because then the tool can do its own "schema" migration. For example, we so this for pytest in the setup.cfg. |
||
|
||
`tox -r` will execute tests for the Python versions specified in `setup.cfg <https://github.com/blue-yonder/tsfresh/blob/1297c8ca5bd6f8f23b4de50e3f052fb4ec1307f8/setup.cfg>`_ using the `envlist` variable. For example, if `envlist` is set to `py37, py38`, the test suite will run for Python 3.7 and 3.8 on the local development platform, assuming the binaries for those versions are available locally. The exact Python microversions (e.g. `3.7.1` vs `3.7.2`) depend on what is installed on the local development machine. | ||
This will execute tests for the Python versions specified in `setup.cfg <https://github.com/blue-yonder/tsfresh/blob/1297c8ca5bd6f8f23b4de50e3f052fb4ec1307f8/setup.cfg>`_ using the `envlist` variable. For example, if `envlist` is set to `py37, py38`, the test suite will run for Python 3.7 and 3.8 on the local development platform, assuming the binaries for those versions are available locally. The exact Python microversions (e.g. `3.7.1` vs `3.7.2`) depend on what is installed on the local development machine. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, did not spot this in the last PR: can you make sure the link points to the latest HEAD/main version of the file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Responded to feedback here 3ddad59 This might result in dead links in the future e.g. if we change the file and not a great way to trace them. If we pin to a specific commit even if the file changes, it will be more traceable as to what happened, instead of a 404. |
||
|
||
A recommended way to manage multiple Python versions when testing locally is with `pyenv`, which enables organized installation and switching between versions. | ||
|
||
In addition to running tests locally, you can also run them in a Dockerized testing environment: | ||
|
||
|
||
.. code:: | ||
|
||
make test-all-testenv | ||
|
||
|
||
This command will initially take some time. However subsequent invokations will be faster, and testing this way ensures a clean, consistent test environment, regardless of your local setup. | ||
|
||
|
||
Documentation | ||
''''''''''''' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to specify also the patch version number? I do not want to create the need to update this file whenever there is a new python version coming out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3ddad59
I see all I need to do is specify major.minor like 3.10 and pyenv will interpret this as latest.