Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Aug 7, 2018

This pull request includes a fix for the AttributeError: module 'enum' has no attribute 'IntFlag' error which would appear when using python3 for a pack virtualenv (pack was created using use_python3=true parameter) and running on RHEL / CentOS (see https://forum.stackstorm.com/t/unable-to-run-an-action-implemented-by-python3/217/7 for details).

The issue was related to enum Python package being installed in python2.7 virtual environment. The python runner action script would try to use that package instead of enum module from Python 3 stdlib.

The fix includes adding <virtualenv path>/lib/python3.6 directory to PYTHONPATH for Python runner actions. This way we ensure Python 3 stdlib imports always have precedence over other python2.7 site-packages imports.

Kami added 2 commits August 7, 2018 12:16
for python runner actions for packs which were created with
"use_python3=true" flag.

This way we ensure modules and packages from Python3 directory are used
before modules and packages from python2.7 site-packages directory.

This way we avoid issues such as the one with the enum import.
@Kami Kami added this to the 2.9.0 milestone Aug 7, 2018
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Please consider adding some comments in the code about how this situation happens to begin with, not just what the fix is itself. I'm not 100% sure why you are fixing this the way you are, so if I'm 100% off base with the rest of my review, please disregard.

Okay, so the enum library is getting loaded from someplace it shouldn't be. Sounds like it's getting loaded from the system's Python environment. Is that correct? In that case, wouldn't a better fix be to create the pack's virtualenv with the --no-site-packages flag? That's been the default flag for virtualenv for quite some time now.

Or if the pack's requirements.txt includes enum because the pack was developed with Python 2 (meaning pip freeze would include the enum library), but run with Python 3, the site-packages would be searched before the built-in library. But in that case, using the enum; python_version <= 2.7 environment markers from PEP 508 might be a better fix, although it requires pip > 6.0 (which we do anyway, I think) and it requires pack developers to know how to use the option and update their requirements.txt appropriately.

If neither of those are better solutions, this seems reasonable. I do want to point out that this PR does make it impossible for PyPI packages to "override" any of the built-in packages. For instance: if I had a special JSON library that was backwards compatible with the built-in json library it could supply a json module in site-packages so code that imported json would automatically use my library instead of the standard library module. But I can't think of any libraries off the top of my head that do that, and I don't know that we necessarily want to support them if we find ones that do.

So if you think that this is a more reasonable fix for the problem than the solutions above, I approve of it.

@Kami
Copy link
Member Author

Kami commented Aug 9, 2018

Okay, so the enum library is getting loaded from someplace it shouldn't be. Sounds like it's getting loaded from the system's Python environment. Is that correct?

Yes, it's getting loaded from system Python 2.7 virtual environment which is used by all the StackStorm components.

In that case, wouldn't a better fix be to create the pack's virtualenv with the --no-site-packages flag? That's been the default flag for virtualenv for quite some time now.

Yes, that would be ideal. In fact, it's something I tried to do in the past, but sadly our components cross-depend on st2common too much so it's very hard to achieve that without a massive refactor.

Right now Python runner actions and action runner depends on st2actions and st2common. st2common depends on various dependencies which are installed into system virtual environment (including enum).

So the easiest way to make pack virtual environments work is to simply inherit all the system dependencies.

We do that by manipulating PYTHONPATH instead of using --system-site-packages and copying / symlinking dependencies over. Symlinking causes issues on some distros and hard copying dependencies over would mean wasting a lot of space and each time StackStorm gets upgraded, all the virtualenvs would need to be rebuilt.

Or if the pack's requirements.txt includes enum because the pack was developed with Python 2 (meaning pip freeze would include the enum library), but run with Python 3, the site-packages would be searched before the built-in library. But in that case, using the enum; python_version <= 2.7 environment markers from PEP 508 might be a better fix, although it requires pip > 6.0 (which we do anyway, I think) and it requires pack developers to know how to use the option and update their requirements.txt appropriately.

That's what happens right now - enum is only installed when running under Python 2.7 (system StackStorm virtual environment), but as mentioned above and below, main problem is "mixing" those dependencies together.

In short - this is a workaround and the main problem is mixing Python 2.7 and 3 dependencies. For the most part that is not an issue since all other libraries we use work with Python 2.7 and 3.x, but enum is a special case (it's needed by 2.7 where it's a package and it's included in 3.x stdlib where it's a module).

All of that should hopefully become less relevant once we fully support Python 3 end to end (then this won't be an issue anymore).

@Kami Kami merged commit 5c80580 into master Aug 9, 2018
@Kami Kami deleted the python3_import_issue_fix branch August 9, 2018 14:01
@blag
Copy link
Contributor

blag commented Aug 9, 2018

Thanks for the additional explanation - all of that sounds reasonable.

@blag blag mentioned this pull request Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants