s0nea commented 3 years ago

I'm not really sure if it's actually a bug and Python 3.7+ is required (and the issue can just be closed). Just in case it's not, I like to raise the issue here. So... on my local environment I noticed the following issue when running tox:

tox
py3 installed: aiofiles==0.6.0,attrs==20.3.0,click==7.1.2,dataclasses==0.8,fastapi==0.63.0,h11==0.12.0,importlib-metadata==3.10.0,iniconfig==1.1.1,packaging==20.9,pluggy==0.13.1,py==1.10.0,pydantic==1.7.3,pyfakefs==4.4.0,pyparsing==2.4.7,pytest==6.2.2,pytest-asyncio==0.14.0,pytest-datadir==1.3.1,pytest-mock==3.5.1,starlette==0.13.6,toml==0.10.2,typing-extensions==3.7.4.3,uvicorn==0.13.3,websockets==8.1,zipp==3.4.1
py3 runtests: PYTHONHASHSEED='2579776963'
py3 runtests: commands[0] | pytest gravel/
============================================================================================================ test session starts ============================================================================================================
platform linux -- Python 3.6.12, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /.../src/aquarium/src, configfile: tox.ini
plugins: datadir-1.3.1, pyfakefs-4.4.0, asyncio-0.14.0, mock-3.5.1
collected 17 items                                                                                                                                                                                                                          

gravel/tests/unit/cephadm/test_cephadm.py .......                                                                                                                                                                                     [ 41%]
gravel/tests/unit/controllers/test_config.py ...                                                                                                                                                                                      [ 58%]
gravel/tests/unit/controllers/test_gstate.py .F                                                                                                                                                                                       [ 70%]
gravel/tests/unit/controllers/orch/test_ceph.py .....                                                                                                                                                                                 [100%]

================================================================================================================= FAILURES ==================================================================================================================
_______________________________________________________________________________________________________________ test_tickers ________________________________________________________________________________________________________________

gstate = <gravel.controllers.gstate.GlobalState object at 0x7fdae58bfb00>

    @pytest.mark.asyncio
    async def test_tickers(gstate):
        from gravel.controllers.gstate import Ticker

        class TestTicker(Ticker):
            def __init__(self, name):
                super().__init__(name, 1.0)
                self.has_ticked = False

            async def _do_tick(self) -> None:
                self.has_ticked = True

            async def _should_tick(self) -> bool:
                return not self.has_ticked

        ticker = TestTicker("test")
        assert "test" in gstate.tickers

>       await gstate._do_ticks()  # pyright: reportPrivateUsage=false

gravel/tests/unit/controllers/test_gstate.py:31: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <gravel.controllers.gstate.GlobalState object at 0x7fdae58bfb00>

    async def _do_ticks(self) -> None:
        for desc, ticker in self.tickers.items():
            logger.debug(f"tick {desc}")
>           asyncio.create_task(ticker.tick())
E           AttributeError: module 'asyncio' has no attribute 'create_task'

gravel/controllers/gstate.py:147: AttributeError
========================================================================================================== short test summary info ==========================================================================================================
FAILED gravel/tests/unit/controllers/test_gstate.py::test_tickers - AttributeError: module 'asyncio' has no attribute 'create_task'
======================================================================================================= 1 failed, 16 passed in 0.36s ========================================================================================================
ERROR: InvocationError: '/.../src/aquarium/src/.tox/py3/bin/pytest gravel/'

jhesketh commented 3 years ago

So yes, the way we're using asyncio means we're only 3.7+ compatible.

We need to choose what min-version we want to support and maybe add a simple startup check for min-version. My feeling is that we should require > 3.8 unless there is a reason not to?

kshtsk commented 3 years ago

@jhesketh so what should be the guide how to setup development environment for those who has 3.6 only on their OS, for example, Leap and CentOS?

kshtsk commented 3 years ago

Even next version of Leap 15.3 will not have python 3.7+ from the box.

jhesketh commented 3 years ago

Hmm, that is perhaps a challenge.

My understanding is that we will only be providing/supporting Aquarium on MicroOS. Thus we only really need to test in that environment - which happens to be python 3.8.

So could we just require a developer to run 3.8 (either from tumbleweed or a different distro etc)?

The developer experience is important too though, and providing a simple way to test for the average developer would be useful. However, where do we draw the line? What if a developer is running 3.5, do we need to accommodate them?

My feeling is that perhaps supporting 3.6 just for the developer experience is a reasonable compromise. I'd like to hear other opinions on our support matrix and the support for developers sake vs technical.

kshtsk commented 3 years ago

I don't know which system is having the 3.5. I don't think we need to support EOL distro versions. As for openSUSE Leap and Tumbleweed both have python3.6, but no 3.5.

jhesketh commented 3 years ago

It was just an example to highlight the complexity we take on in supporting more than what is necessary and how do we choose an arbitrary line. But yes, I agree, 3.6 makes sense.

jecluis commented 3 years ago

I'm not entirely sure how much of python 3.8's features are being used, so I'm weary of committing to 3.6. The truth is, even for the developer's experience, I would have an expectation that Aquarium is actually run from a VM and not on the host system (you know, because there's this thing about consuming disks). I'm missing the point of being too concerned about requiring 3.8.

kshtsk commented 3 years ago

@jecluis main point for me is possibility to run unit and functional tests on your development machine while being in your favourite editors etc., not to be forced to deploy code before doing anything on some virtualised environment.

jecluis commented 3 years ago

So, few things that are apparent from reading the release notes of 3.7 and 3.8 that are going to mess with the backend, that are not in 3.6:

lack of support for type annotation forward references
lack of asyncio.run()
lack of asyncio.create_task()
lack of asyncio.get_running_loop()
although we're not currently using it, Path.is_mount() is likely to be helpful in the coming weeks
several asyncio optimizations -- note that the whole of gravel relies on asyncio.
and, last but not least, https://github.com/martyanov/aetcd3 only supports 3.8+ -- and this is the only reasonably recent asyncio etcd3 library around, even though it's alpha.

jecluis commented 3 years ago

would it be sufficient if we added documentation to use pyenv for those on a distro without 3.8+?

jhesketh commented 3 years ago

Yep, I think pyenv is the right solution here. This will allow us to ensure developers and CI are using an expected and supported version (probably whatever is in microos).

kshtsk commented 3 years ago

would it be sufficient if we added documentation to use pyenv for those on a distro without 3.8+?

I don't think I understand, could you please elaborate how to use pyenv, when you have no corresponding python version compiled on your system?

kshtsk commented 3 years ago

Collecting pyenv
  Using cached pyenv-0.0.1.tar.gz (1.4 kB)
Building wheels for collected packages: pyenv
  Building wheel for pyenv (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/kyr/kshtsk/aquarium/p/bin/python -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-_vdtl82a/pyenv_0ca75ea7a0e24e46bf4f333c7441d2ce/setup.py'"'"'; __file__='"'"'/tmp/pip-install-_vdtl82a/pyenv_0ca75ea7a0e24e46bf4f333c7441d2ce/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-fi_koj2x
       cwd: /tmp/pip-install-_vdtl82a/pyenv_0ca75ea7a0e24e46bf4f333c7441d2ce/
  Complete output (12 lines):
  running bdist_wheel
  running build
  installing to build/bdist.linux-x86_64/wheel
  running install
  ############################ NOTE ############################
  We are sorry, but this package is not installable with pip.

  Please read the installation instructions at:

  https://github.com/pyenv/pyenv#installation
  ##############################################################

  ----------------------------------------
  ERROR: Failed building wheel for pyenv
  Running setup.py clean for pyenv
Failed to build pyenv

jhesketh commented 3 years ago

I've not installed pyenv from pip before.. I'm not sure it's possible looking at that message. But it's in zypper (zypper in pyenv).

So I guess our requirement for developers will be to have pyenv installed on their system.

kshtsk commented 3 years ago

@jhesketh Looks like system pyenv does not provide any released python version of 3.8+ on Leap 15.2:

> pyenv install --list | grep 3\.8
  3.8-dev
  miniconda-3.8.3
  miniconda3-3.8.3

kshtsk commented 3 years ago

However for pyenv installed from the source code there are more options:

pyenv install --list | grep 3\.8
  3.8.0
  3.8-dev
  3.8.1
  3.8.2
  3.8.3
  3.8.4
  3.8.5
  3.8.6
  3.8.7
  3.8.8
  3.8.9
  miniconda-3.8.3
  miniconda3-3.8.3
  miniconda3-3.8-4.8.2
  miniconda3-3.8-4.8.3
  miniconda3-3.8-4.9.2

jhesketh commented 3 years ago

Hmm, that's unfortunate. I wonder if it's due to the version of pyenv that you might have. On tumbleweed I have lots more versions available.

Could you try installing pyenv from git: https://github.com/pyenv/pyenv#basic-github-checkout

We might need to give a min version of pyenv to get a min version of python, and if your distro doesn't have the min version of pyenv you need to get it from github?

kshtsk commented 3 years ago

Hmm, that's unfortunate. I wonder if it's due to the version of pyenv that you might have. On tumbleweed I have lots more versions available.

Could you try installing pyenv from git: https://github.com/pyenv/pyenv#basic-github-checkout

Have you read my comment regarding pyenv installed from the source code?

We might need to give a min version of pyenv to get a min version of python, and if your distro doesn't have the min version of pyenv you need to get it from github?

jhesketh commented 3 years ago

Have you read my comment regarding pyenv installed from the source code?

Yes, that looked like you were trying to install it from pypi though (you didn't share the commands used). Have you tried using the binaries that are in the pyenv git as per my last link?

kshtsk commented 3 years ago

Have you read my comment regarding pyenv installed from the source code?

Yes, that looked like you were trying to install it from pypi though (you didn't share the commands used). Have you tried using the binaries that are in the pyenv git as per my last link?

No, "from source code" means go to the GitHub, read the doc, and do some commands:

 git clone https://github.com/pyenv/pyenv.git ~/.pyenv
(cd ~/.pyenv && src/configure && make -C src)
echo 'export PYENV_ROOT="$HOME/.pyenv"' >> ~/.bash_profile
echo 'export PATH="$PYENV_ROOT/bin:$PATH"' >> ~/.bash_profile

kshtsk commented 3 years ago

If you install on Leap 15.2 via zypper, the system pyenv is too old and does not have needed python 3.8, however when you install it pyenv from github, it looks like has latest python versions available. (installing pyenv via pip does not work at all).

jhesketh commented 3 years ago

Right, so from github you can get newer python versions now?

kshtsk commented 3 years ago

Right, so from github you can get newer python versions now?

yes and this looks so hackwise

jhesketh commented 3 years ago

well really what we're saying is the min/required version of python is 3.8.x. This is just one way to get it. We can give just a few bash commands for a dev to get started. Or, if they know what they are doing, they can get it from their distro, 3rd party repos, or many other locations.

kshtsk commented 3 years ago

Few? It is way more, you need to install dependencies in order to build pyenv and the needed python version, and we would be lucky it compiled without errors :-)

jhesketh commented 3 years ago

we could try pointing to the pyenv installer I just noticed: https://github.com/pyenv/pyenv-installer

kshtsk commented 3 years ago

Did you mean this repo https://download.opensuse.org/repositories/devel:/languages:/python:/Factory/openSUSE_Leap_15.2/x86_64/

jhesketh commented 3 years ago

No.

But if you want to install python from elsewhere, that's fine. Again, we're just proposing that we set a required python version. How you get that is up to you. As a simple way we can point people at pyenv.

kshtsk commented 3 years ago

It is really not simple, unless we support this, to be honest we can not ask users to use any version of python either it is pyenv or some external if we do not regular test this setup, otherwise it become a nightmare for user to try and setup the environment.

kshtsk commented 3 years ago

In summary, if we strict to use 3.8 we should say we do not support any system which does not have 3.8 by default as a development environment. And this should be at the beginning of FZTH doc, so people do not waste their time.

kshtsk commented 3 years ago

Again, for example, leap does not have a standard way of installing python 3.8, which means it is unsupported and people are left to their owns when trying to use this environment.

tserong commented 3 years ago

I wonder if we're conflating two things here... Does anyone actually run aquarium on their dev host (which might have the wrong python version, and of course will have problems with wrong system librados as Kyr mentioned in https://github.com/aquarist-labs/aquarium/pull/405#issuecomment-818300088)? Or is it only ever run inside the Vagrant machine (which will always have a supported python version)?

What do we, as those hacking on this thing, actually need to do?

Write and edit code (any python 3 version is fine)
Run unit tests (must have python 3.8+, but tests don't seem to actually import rados, so it doesn't matter if system librados is wrong)
Build images (any python 3 version is probably fine - we know it works at least on 3.6 and 3.8)
Run aquarium (must have python 3.8+, and working system librados), but that always happens inside one of our images, whether it's on a dev machine or in Jenkins or whatever, so all the right pieces are in place.

https://github.com/aquarist-labs/aquarium/pull/403 brings in aetcd3, which explicitly requires python >= 3.8, so breaks the leap jenkins build during setup-dev, but I wonder if maybe we can rework the setup-dev and build-image tooling, so that it's possible to build images on any python3 system, but do actual hacking only on python 3.8 systems. That would mean if you're on a python3.8 host, you can do whatever you want, but if you're on a python3.6 host, you have to first build an image, then do all your hacking from inside the VM. Would that give the best of both worlds?

jecluis commented 3 years ago

* Write and edit code (any python 3 version is fine)

This may be affected by the python env, depending on your editor of choice. For instance, vscode relies on either the system python or your existing venv. If your venv only supports 3.6, vscode is going to have a hard time figuring out 3.8 constructs that do not exist in your libraries. This, along with running tox, seems to be where pyenv could come in.

jhesketh commented 3 years ago

Again, for example, leap does not have a standard way of installing python 3.8, which means it is unsupported and people are left to their owns when trying to use this environment.

Yep, but this is the approach I would take. Only 3.8 is supported, but we can still point developers at some useful tools like pyenv if they're interested.

As an aside, I'm also beginning to doubt the usefulness of testing on leap. Maybe we just drop that test.

kshtsk commented 3 years ago

What do we, as those hacking on this thing, actually need to do?

Write and edit code (any python 3 version is fine)

Run unit tests (must have python 3.8+, but tests don't seem to actually import rados, so it doesn't matter if system librados is wrong)

The fact that 'import rados' is not actually loaded at the moment only means that current coverage is low, and it does not mean we will not extend it. Anyways it looks like trap, which is not obvious.

Build images (any python 3 version is probably fine - we know it works at least on 3.6 and 3.8)

Run aquarium (must have python 3.8+, and working system librados), but that always happens inside one of our images, whether it's on a dev machine or in Jenkins or whatever, so all the right pieces are in place.

403 brings in aetcd3, which explicitly requires python >= 3.8, so breaks the leap jenkins build during setup-dev, but I wonder if maybe we can rework the setup-dev and build-image tooling, so that it's possible to build images on any python3 system, but do actual hacking only on python 3.8 systems. That would mean if you're on a python3.8 host, you can do whatever you want, but if you're on a python3.6 host, you have to first build an image, then do all your hacking from inside the VM. Would that give the best of both worlds?

I was able to setup pyenv venv and run unitest within it, however I have to caveats:

Librados as I mentioned above
The actual setup is quite different from the current and requires: extra system dependencies to build pyenv (because system pyenv is too old and does not fit our needs), maybe requires more dependencies to build some other python package which we use from system python.

kshtsk commented 3 years ago

Again, for example, leap does not have a standard way of installing python 3.8, which means it is unsupported and people are left to their owns when trying to use this environment.

Yep, but this is the approach I would take. Only 3.8 is supported, but we can still point developers at some useful tools like pyenv if they're interested.

As an aside, I'm also beginning to doubt the usefulness of testing on leap. Maybe we just drop that test.

It is absolutely useful if we declare we support long term openSUSE releases as a development environment, otherwise we should print with big bold letters, USE openSUSE Tumbleweed :-D

tserong commented 3 years ago

Librados as I mentioned above

I think we can forget about this. AFAICT we don't actually need it on the host, it's only needed at runtime in the image. I suspect the fact that setup-dev.sh checks for it is an artifact of very early development, before we had the images, when aquarium was being run experimentally on the host. TL;DR: it's probably fine to do something like this:

--- a/tools/setup-dev.sh
+++ b/tools/setup-dev.sh
@@ -6,7 +6,6 @@ dependencies_opensuse_tumbleweed=(
   "kpartx"
   "make"
   "python3"
-  "python3-rados"
   "python3-kiwi"
   "nodejs-common"
   "npm"
@@ -21,7 +20,6 @@ dependencies_debian=(
   "make"
   "python3"
   "python3-pip"
-  "python3-rados"
   "python3-kiwi"
   "python3-kiwi-boxed-plugin"
   "python3-venv"
@@ -36,7 +34,6 @@ dependencies_ubuntu=(
   "make"
   "python3"
   "python3-pip"
-  "python3-rados"
   "python3-kiwi"
   "python3-kiwi-boxed-plugin"
   "python3-venv"
@@ -182,11 +179,6 @@ if [ $PY_MINOR -lt 8 ] ; then
   exit 1
 fi

-if ! ( echo "import rados" | python3 &>/dev/null ); then
-  echo "error: missing rados python bindings"
-  exit 1
-fi
-
 if ! npm --version &>/dev/null ; then
   echo "error: missing npm"
   exit 1
@@ -208,9 +200,11 @@ if [ -d venv ] ; then
     echo
 fi

-# we need system site packages because librados python bindings appear to only
-# be available as a package. It might be a good idea to compile it from the
-# ceph repo we keep as a submodule, but it might be overkill at the moment?
+# We need system site packages because librados python bindings are only
+# available as a package, they're not on pypi (and Tim thinks they probably
+# never will be). Setting --system-site-packages here means the venv will
+# work properly if someone uses it inside the image, where we need to be
+# able to fall back to system packages to get librados.
 python3 -m venv --system-site-packages venv || exit 1

 source venv/bin/activate

tserong commented 3 years ago

431 adds the patch from the above comment. We should probably also add some notes about pyenv to docs/fzth, but other than that, I think we've probably nailed this one, unless anyone can think of anything else that's missing?

jecluis commented 3 years ago

I'm good with closing this one. @jhesketh @kshtsk @s0nea anyone opposed?

jhesketh commented 3 years ago

No objection from me.

s0nea commented 3 years ago

No, nothing from my side. Thank you!

jecluis commented 3 years ago

Alright, closing. Let's open a different ticket should any new issues arise.

aquarist-labs / aquarium

gravel: requires Python 3.7+ ? #361

431 adds the patch from the above comment. We should probably also add some notes about pyenv to docs/fzth, but other than that, I think we've probably nailed this one, unless anyone can think of anything else that's missing?