Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

tonybaloney · 2018-01-01T03:43:41Z

As part of #3233 the pack (and/or) the user needs to be able to specify the minimum Python version required for the pack to operate.

In the case of Python 3, this requires another Python binary in the config.

This PR adds support for the st2common.virtualenv module to specify another Python binary path when building the env.

st2.conf now has a python3_binary setting to specify the default Python 3 binary path. This defaults to /usr/bin/python3 in both Ubuntu and Redhat. For newer versions of ubuntu, /usr/bin/python3 is a symlink to python3.5.

Python 3 has to be explicitly specified in the pack meta file.

If the user does not have a Python 3 binary installed it will fail on Pack Installation, virtualenv will raise an error.

https://github.com/StackStorm/st2/pull/3922/files#diff-4572714cfd795436cbea16dff51e3231R18

The configuration is such:

[actionrunner]
...
# Python binary which will be used by Python actions.
python_binary = /data/stanley/virtualenv/bin/python
python3_binary = /data/stanley/virtualenv/bin/python

The configuration of Python 2 binaries would default to the one running the script, but since this won't be the case in 3, it defaults again to /usr/bin/python3 https://github.com/StackStorm/st2/pull/3922/files#diff-f7f00f2d9fa85d6b39dc2def0f872561R222

pack.yaml now has a flag to specify the creation of the virtualenv using a Python 3 binary

system:
   python3: true

TODO :

write tests to check the integration
update pack.yaml meta template

Notes:

The dev Makefile now explicitly specifies python 2.7 in the virtual env. Some modern distributions will default to 3.x
https://github.com/StackStorm/st2/pull/3922/files#diff-b67911656ef5d18c4ae36cb6741b7965R273

There is now a switch to change the default Python version from 2 to 3. This is not configurable since it would likely break everything! https://github.com/StackStorm/st2/pull/3922/files#diff-1672fdd913c3ff338b3d0f9285d2fa81R59

Packs without meta files would not fail at the virtualenv stage, which is the same behaviour as before https://github.com/StackStorm/st2/pull/3922/files#diff-1672fdd913c3ff338b3d0f9285d2fa81R59

…ython 3 path

…hon3' flag in meta

…e only one that St2 works in

tonybaloney · 2018-01-01T06:07:43Z

Just tested against a dev instance by:

Cloning the libcloud pack
Changing pack.yaml to have python3: true at the bottom
Installing the pack from git
Checking the Python version installed in the virtualenv

(virtualenv) ubuntu@ubuntu-xenial:~/st2$ /opt/stackstorm/virtualenvs/libcloud/bin/python --version
Python 3.5.2

tonybaloney · 2018-01-01T11:27:16Z

Integration tested and added unit tests as well. This is now done.

tonybaloney · 2018-01-01T11:27:40Z

@Kami It's all yours now 🎁

Kami · 2018-01-01T12:50:14Z

st2common/st2common/util/virtualenvs.py

+
+    if has_pack_meta:
+        logger.debug('Checking pack specific Python version.')
+        if 'python3' in pack_meta.keys():


A while ago we added a couple more fields to the pack model and pack.yaml file, notably dependencies and system.

Perhaps it would be better to utilize system field instead of a top-level one (e.g. system.python3 = True or similar).

We also need to decide and clarify what that attribute really means - does it means that pack supports just Python 3 or that it supports Python 3 in addition to Python 3? And once we decide what it means and represents, we should document it :)

Agreed on the documentation. Top-level attribute seems off also, where is the meta file model? I couldn't find it anywhere.
Currently, it means that the pack should use a Python3 virtualenv. So it supports Python 3 at least, but shouldn't fall back to 2

We don't have a schema for system attribute yet, but you can add it and extend it here - https://github.com/StackStorm/st2/blob/master/st2common/st2common/models/api/pack.py#L135

For now, you can just add declaration for system.python3 attribute there under system field.

Kami · 2018-01-01T12:51:49Z

st2common/st2common/util/virtualenvs.py

@@ -73,6 +74,19 @@ def setup_pack_virtualenv(pack_name, update=False, logger=None, include_pip=True
        msg = 'Pack "%s" is not installed. Looked in: %s' % (pack_name, search_paths)
        raise Exception(msg)

+    try:
+        pack_meta = get_pack_metadata(pack_path)


Perhaps, for dependency injection and testing reasons, this function should take use_python3 as an argument instead of reading the metadata file itself.

Thinking out loud - I first thought this might slow down things quite a bit when running st2ctl register --register-setup-virtualenvs flag because we now also need to read and parse metadata file for each pack.

But after some more thought - we already need to do that, so another read and parse shouldn't add too much overhead since file should already be in filesystem cache. Future optimization would perhaps be to refactor the code a bit so we don't need to read and parse metadata file multiple times, but probably not needed right now.

I assumed that the pack would still install if the meta-file wasn't in place, so I didn't want to alter the original behaviour

Metadata is mandatory so it would fail at some other step, but that's not really important here and I'm fine with the current approach.

I was mostly just "thinking out loud" about the potential performance overhead from reading and parsing pack metadata multiple times during install phase :)

Kami · 2018-01-01T12:55:34Z

Nice work 👍

tonybaloney · 2018-01-01T21:32:24Z

Also @Kami is the meta model typed anywhere? It seems a bit shonky to just hardcode the dictionary keys into the virtualenv logic.

tonybaloney · 2018-01-03T00:06:50Z

What do you want me to change? Move the flag to system?

Kami · 2018-01-03T11:22:46Z

Also @Kami is the meta model typed anywhere? It seems a bit shonky to just hardcode the dictionary keys into the virtualenv logic.

It's not, but we can / should add a schema for it.

Kami · 2018-01-03T11:43:09Z

Hopefully my clarification on the comments helped so we can move this forward :)

Another thing which would be nice to have is ability to "force" which Python version to use. The use case would be pack declaring system.python3 = True in metadata, but it turns out pack doesn't actually work completely or in part under Python 3.

Not a blocker right now and another way to approach that would be to "fix" that in the upstream pack, but at least the prerequisite for us merging "system.python3 = True` change for each pack should be that we run tests for that pack under Python 3.

tonybaloney · 2018-01-05T11:10:13Z

@Kami moved the configuration to system.python3. will raise issue to fix st2-setup-tests and a PR for docs

…virtualenv_version

tonybaloney · 2018-01-05T11:39:06Z

@Kami I'm done with this one now. The build should pass now I've run make configgen on a dev installation and pushed the updates back

Kami · 2018-01-05T12:07:17Z

Also, just curious since I haven't done any Python 3 testing myself yet.

Actions are not fully self contained (same goes for sensors) and rely on some common StackStorm code which is only tested and works under Python 2.7.

I assume this could cause issues down the road so we should also add some basic StackStorm CI tests for a dummy / example python 3 pack which exercises all the "common" StackStorm code base, notably datastore functionality which is provided via action / sensor service.

Kami · 2018-01-05T12:13:48Z

After some more thought - as mentioned before, I think it would be much safer and better to start with and also add use_python3 or similar parameter to packs.install and packs.setup_virtualenv action. Then user could force python3 use by running st2 pack install --use-python3 or similar command.

This is much more forgiving than defaulting flag in metadata file to True3. If something doesn't work, user can simply re-install the pack omitting that flag. If there is some edge case and pack specifies system.python3 = True and the pack doesn't work (due to some bug in upstream code, st2 code or user environment), user would need to submit an upstream PR to the pack or hack / modify pack.yaml on disk. Neither of which is really desirable.

So in short, system.python3 attribute could be used to tell user that pack also works under Python 3, but user should still specify a parameter to packs.install / packs.setup_virtualenv to actually use Python 3 binary for that pack.

Kami · 2018-01-05T18:20:00Z

(from Slack)

kami [1:14 PM]
@anthonypjshaw #3922 (comment) sorry for getting back to that, but i'm just worried this could cause bad experience for our users
[1:14]
since our code and packs are not fully tested under python 3
[1:14]
and using the flag could cause a bad experience, while i think explicit opt-in approach is better
[1:14]
we could still utilize this flag as "signaling" user that this pack also works with python 3
[1:14]
but it should be explicit opt-in
[1:15]
and eventually, when we also test more of the stackstorm code base and packs under python 3, we could perhaps do that
[1:17]
@anthonypjshaw if needed and / or wanted, and you agree on that approach, i can also take that over 🙂

@tonybaloney Let me know if you are find with that approach (explicit opt-in via action parameter and CLI flag, until we get the whole CI up and running, etc. system.python3 would just serve informational role, indicating to the user Python 3 is likely also supported by that pack, but user needs to explicit opt-in on install).

If you are fine with that approach, I can make those changes.

tonybaloney · 2018-01-05T21:11:12Z

@Kami in all the scenarios I’m thinking of, it’s the pack author who chooses whether the pack and the actions require Python 3. This could be for a number of reasons, like a package that won’t install in Python 2 (there are more and more of those now) or they used the Python 3 standard library namespaces. Or they wanted to stick to the Python 3 string idioms.
I would also assume the pack author is more aware and more technical than the one installing the pack. Agreed that the testing process needs to change- the pack unit tests need to run under Python 3 where it has been specified. Also, the subset of modules that are shared should also be run under 3 to check regressions.
As an example I have packs where I’ve had to deal with Unicode inbound and encoding which I would clean up in a Python 3 action. That code would not be backward compatible with 2 and the user would not know that either.

Kami · 2018-01-08T19:53:37Z

See #3929 (comment) for somewhat related issue.

I don't feel comfortable merging this with a lot better and more complete test coverage. I'm just worried that without a complete end to end test coverage, it will just represent a liability for us and cause a bad experience and pain for us and users who will try to use it.

In general I think it's better user experience if something is not supported than having something somewhat supported, but then things break and there are many edge cases when trying to use it. That will just frustrate most of the "typical" users (there are of course more technical users who like challenges and fixing things like that, but those are far and between).

tonybaloney · 2018-01-31T02:08:49Z

Considering the amount of effort and changes in better testing Python 3 in core libraries what is the new status of this PR ? @Kami @LindsayHill

Kami · 2018-02-01T20:10:04Z

Sorry, didn't have time to dig in yet and take it across the finish line.

Sadly this PR is just a small part of much bigger change to get it into "fully done" state - first I need to figure out how to finalize this PR so everyone will be happy, then documentation, then hooking it up against StackStorm Exchange CI to make sure tests correctly use Python 3 when needed, etc.

But as far as you original question goes - it will still take some time to get everything running under Python 3 and even when that is the case, we will probably still need change like that to allow users to run Python runner actions under different version of Python (e.g. 3 when platform is using 2 and also vice versa).

m4dcoder · 2018-02-23T19:40:06Z

@Kami What's the plan for this PR?

brainstorm · 2018-02-25T22:58:59Z

It would be rad to have it in place, today I was trying to have ansible 2.5.0rc1 and python3 but failed and found this PR... I think I'll just create a virtualenv with py3 manually for now, but it would be great to have it in, keep the good work @Kami!

Kami · 2018-02-26T07:36:48Z

@brainstorm It's actually @tonybaloney who did the most work on this PR :)

And yeah, manually creating a virtualenv using python3 binary should work for now.

Last time we couldn't come up with a compromise on how to make this work in a robust and straight forward manner and get it merged.

Perhaps a good middle of the ground approach would be to just add use_python3 (or similar) parameter to pack install action and CLI command. This way it would be opt-in and allow and easy way for user to switch back in case there are issues with Python 3.

What do others think?

Kami · 2018-04-10T16:44:52Z

Closing - continuation in #4076.

Hopefully we can resolve some of those issues which were uncovered yesterday and get it shipped in v2.8.0.

tonybaloney added 7 commits January 1, 2018 14:41

add a Python 3 flag to the virtualenv command

e014412

add config option to sample

286517e

add the python 3 flag to the config generator

5158257

fix a mixup with bin names and add the sed command to configure the P…

ed9dab3

…ython 3 path

fix naming issue

a684978

read pack meta when deploying virtualenv. check for existence of 'pyt…

2de634f

…hon3' flag in meta

change 'make requirements' to default to Python 2.7, since that is th…

7e69044

…e only one that St2 works in

tonybaloney changed the title ~~[WIP] Allow a pack (or the user) to specify the Python binary used in the VirtualEnv~~ Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 Jan 1, 2018

Kami self-assigned this Jan 1, 2018

tonybaloney added 3 commits January 1, 2018 22:01

Add a simple unit test

a194a16

Add a python 2 test and fix the original one

9eab587

Fix assertions

48dd14c

Kami reviewed Jan 1, 2018

View reviewed changes

move pack python3 attribute to system namespace

ad1c241

tonybaloney and others added 2 commits January 5, 2018 22:11

Merge branch 'master' into virtualenv_version

ddf6b3e

update config files after running 'make configgen' needs checking

0867a54

tonybaloney mentioned this pull request Jan 5, 2018

Add description of Python 3 behaviours StackStorm/st2docs#677

Closed

Merge branch 'virtualenv_version' of github.com:tonybaloney/st2 into …

624dd57

…virtualenv_version

Merge branch 'master' into virtualenv_version

2e4e080

Merge branch 'master' into virtualenv_version

c9a5716

Kami mentioned this pull request Feb 28, 2018

Add new --python3 flag to st2 pack install CLI command #4016

Merged

1 task

Kami mentioned this pull request Apr 10, 2018

Add support for Python 3 for Python action runner virtual environments #4076

Closed

Kami closed this Apr 10, 2018

Kami added the python3 label May 28, 2018

This was referenced May 28, 2018

[WIP] Experimental and opt-in Python 3 support for Python runner actions #4148

Closed

Experimental and opt-in Python 3 support for Python runner actions #4149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

tonybaloney commented Jan 1, 2018 •

edited

Loading

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

Kami Jan 1, 2018

tonybaloney Jan 1, 2018

Kami Jan 3, 2018

Kami Jan 1, 2018

Kami Jan 1, 2018

tonybaloney Jan 1, 2018

Kami Jan 3, 2018

Kami commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 3, 2018

Kami commented Jan 3, 2018

Kami commented Jan 3, 2018

tonybaloney commented Jan 5, 2018

tonybaloney commented Jan 5, 2018

Kami commented Jan 5, 2018

Kami commented Jan 5, 2018

Kami commented Jan 5, 2018 •

edited

Loading

tonybaloney commented Jan 5, 2018

Kami commented Jan 8, 2018

tonybaloney commented Jan 31, 2018

Kami commented Feb 1, 2018

m4dcoder commented Feb 23, 2018

brainstorm commented Feb 25, 2018

Kami commented Feb 26, 2018

Kami commented Apr 10, 2018

Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

Allow a pack to specify Python 3 and have corresponding virtualenvs use Python 3 #3922

Conversation

tonybaloney commented Jan 1, 2018 • edited Loading

TODO :

Notes:

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

Kami Jan 1, 2018

Choose a reason for hiding this comment

tonybaloney Jan 1, 2018

Choose a reason for hiding this comment

Kami Jan 3, 2018

Choose a reason for hiding this comment

Kami Jan 1, 2018

Choose a reason for hiding this comment

Kami Jan 1, 2018

Choose a reason for hiding this comment

tonybaloney Jan 1, 2018

Choose a reason for hiding this comment

Kami Jan 3, 2018

Choose a reason for hiding this comment

Kami commented Jan 1, 2018

tonybaloney commented Jan 1, 2018

tonybaloney commented Jan 3, 2018

Kami commented Jan 3, 2018

Kami commented Jan 3, 2018

tonybaloney commented Jan 5, 2018

tonybaloney commented Jan 5, 2018

Kami commented Jan 5, 2018

Kami commented Jan 5, 2018

Kami commented Jan 5, 2018 • edited Loading

tonybaloney commented Jan 5, 2018

Kami commented Jan 8, 2018

tonybaloney commented Jan 31, 2018

Kami commented Feb 1, 2018

m4dcoder commented Feb 23, 2018

brainstorm commented Feb 25, 2018

Kami commented Feb 26, 2018

Kami commented Apr 10, 2018

tonybaloney commented Jan 1, 2018 •

edited

Loading

Kami commented Jan 5, 2018 •

edited

Loading