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

Bump minimum Python version to 3.8 #934

Merged
merged 4 commits into from
Mar 23, 2020

Conversation

danielmitterdorfer
Copy link
Member

@danielmitterdorfer danielmitterdorfer commented Mar 20, 2020

With this commit we require at least Python 3.8 for Rally. This is done
as a preparation for Rally's new load generator which requires certain
Python 3.8 features (contextvars, madvise) and uses asyncio internally
which has been improved in more recent versions of Python.

With this commit we require at least Python 3.7 for Rally. This is done
as a preparation for Rally's new load generator which requires certain
Python 3.7 features (like contextvars) and uses asyncio internally which
has been improved in more recent versions of Python.
@danielmitterdorfer danielmitterdorfer added :misc Changes that don't affect users directly: linter fixes, test improvements, etc. breaking Non-backwards compatible change labels Mar 20, 2020
@danielmitterdorfer danielmitterdorfer added this to the 1.5.0 milestone Mar 20, 2020
@danielmitterdorfer danielmitterdorfer self-assigned this Mar 20, 2020
@danielmitterdorfer danielmitterdorfer added the enhancement Improves the status quo label Mar 20, 2020
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thanks! I left a comment for linking (and making it a required item) the steps to install the compilation prerequisites.

::
# select that version for the current user
# see https://github.com/pyenv/pyenv/blob/master/COMMANDS.md#pyenv-global for details
pyenv global 3.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially I wondered if this is too invasive i.e. will change the default version of Python for the user leading to surprises.

However, as these instructions are targeted also for released versions, I think this is ok. Otherwise we'd suggest something like:

mkdir some_stuff_rally_dir
cd some_stuff_rally_dir
pyenv local 3.8.0
exec $SHELL

# and then install Rally from within this dir
pip3 install esrally


# Install Python
pyenv install 3.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention specifically the system package prerequisites(for compiling python) as mandatory? These are listed nicely here: https://github.com/pyenv/pyenv/wiki/common-build-problems#prerequisites

Without the packages present, one can hit issues like:

$ pip3 install esrally
Collecting esrally
  Using cached https://files.pythonhosted.org/packages/eb/ca/371dcc49c7abcdb1bc8fec8aa76467658c891d935f0c2967137ff036a324/esrally-1.4.1-py3-none-any.whl
Collecting py-cpuinfo==3.2.0 (from esrally)
  Downloading https://files.pythonhosted.org/packages/48/1d/404dff45f8bdc63063ac0b822b557a5a5df54679de55261021efc882b8dd/py-cpuinfo-3.2.0.tar.gz (76kB)
     |████████████████████████████████| 81kB 4.7MB/s
    ERROR: Command errored out with exit status 1:
     command: /home/vagrant/.pyenv/versions/3.8.0/bin/python3.8 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-h35_rza2/py-cpuinfo/setup.py'"'"'; __file__='"'"'/tmp/pip-install-h35_rza2/py-cpuinfo/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base pip-egg-info
         cwd: /tmp/pip-install-h35_rza2/py-cpuinfo/
    Complete output (11 lines):
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/home/vagrant/.pyenv/versions/3.8.0/lib/python3.8/site-packages/setuptools/__init__.py", line 20, in <module>
        from setuptools.dist import Distribution, Feature
      File "/home/vagrant/.pyenv/versions/3.8.0/lib/python3.8/site-packages/setuptools/dist.py", line 35, in <module>
        from setuptools import windows_support
      File "/home/vagrant/.pyenv/versions/3.8.0/lib/python3.8/site-packages/setuptools/windows_support.py", line 2, in <module>
        import ctypes
      File "/home/vagrant/.pyenv/versions/3.8.0/lib/python3.8/ctypes/__init__.py", line 7, in <module>
        from _ctypes import Union, Structure, Array
    ModuleNotFoundError: No module named '_ctypes'
    ----------------------------------------
ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.

which are a bit hard to diagnose.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pondered doing this but this is also linked from Pyenv's installation instructions which is the reason I thought of not replicating it here. But you have a point...

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 60193d8

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'd personally emphasize with bold as below, but it's a personal preference (to ensure maximum visibility for users)

and **ensure that all of** `pyenv's prerequisites <https://github.com/pyenv/pyenv/wiki/common-build-problems#prerequisites>`_ are installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 726f890.

danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this pull request Mar 20, 2020
This will be properly solved in elastic#934.
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM. Left an additional comment (personal preference, up to you).

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

LGTM, I agree w @dliappis , as sometimes people tend to breeze over install instructions and seeing things in bold definitely is eye catching

@danielmitterdorfer danielmitterdorfer changed the title Bump minimum Python version to 3.7 Bump minimum Python version to 3.8 Mar 23, 2020
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM. Let's do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Non-backwards compatible change enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants