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

Python3 compatibility #1673

Merged
merged 17 commits into from Oct 4, 2018

Conversation

Projects
None yet
6 participants
@SteVwonder
Copy link
Member

SteVwonder commented Sep 23, 2018

This PR builds off of @trws's PR and updates the python bindings to work for both python 2 & 3.

Major changes include:

  • Any strings returned by the python bindings are now unicode strings. Most char*s coming out of Flux are assumed to be utf-8 minus a few exceptions (e.g., job states from the JSC).
  • Any strings passed to the bindings are checked to see if they are unicode strings, if so, they are encoded as utf-8. If they are raw bytes, they are passed straight through.
  • Moves the python bindings tests to t/python from src/bindings/python/tests.
  • Switches the tests from using the sideflux to using the subflux module
  • Adds tests for the jsc bindings and unicode support.

Choosing which python to build against

The current automake macro searches automatically for a python interpreter >= 2.7. AFAIK, it searches from the newest, stable python version (3.7) and works its way down (to 2.1). To override this behavior, users can set the PYTHON or PYTHON_VERSION env variables when running configure (these two vars are documented in the output of configure --help). After some searching online, I didn't see any pre-built solutions for compiling the bindings with both python2 and python3 simultaneously with autotools. It seems like a lot of projects that do this treat their bindings as separate project or a sub-project housed in-tree. I am starting a discussion on how to handle this in #1448.

Closes #1669
Closes #1448
Closes #1046
Closes #805

@SteVwonder SteVwonder requested a review from trws Sep 23, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 23, 2018

Coverage Status

Coverage decreased (-0.04%) to 79.961% when pulling 8d88e62 on SteVwonder:python3-compatibility into 8b195da on flux-framework:master.

@SteVwonder SteVwonder force-pushed the SteVwonder:python3-compatibility branch 2 times, most recently from 35eb00d to 5626853 Sep 23, 2018

@SteVwonder SteVwonder force-pushed the SteVwonder:python3-compatibility branch 13 times, most recently from 8310bd9 to 3871db9 Sep 24, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 29, 2018

Codecov Report

Merging #1673 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1673      +/-   ##
==========================================
- Coverage    79.3%   79.28%   -0.03%     
==========================================
  Files         186      186              
  Lines       35059    35063       +4     
==========================================
- Hits        27804    27798       -6     
- Misses       7255     7265      +10
Impacted Files Coverage Δ
src/modules/pymod/py_mod.c 0% <0%> (ø) ⬆️
src/common/libflux/mrpc.c 85.49% <0%> (-1.15%) ⬇️
src/common/libflux/reactor.c 91.78% <0%> (-0.92%) ⬇️
src/broker/broker.c 76% <0%> (-0.45%) ⬇️
src/cmd/flux-module.c 85.28% <0%> (-0.31%) ⬇️
src/broker/module.c 83.78% <0%> (-0.28%) ⬇️
src/modules/connector-local/local.c 74.51% <0%> (+1.35%) ⬆️
@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Sep 29, 2018

Finally all green! Ready for review once you are back, @trws.

Sorry about the order in which the commits show up in GitHub. I guess it orders them by the commit date rather than their order in the git history. Unfortunately, that might make this PR a little confusing to review, if you prefer to go through PRs commit-by-commit.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 29, 2018

Cool! I'm really wondering now what is going in Travis. The builds on master keep failing with some kind of output truncation error, but your PR on top of current master worked. 😕

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 2, 2018

This looks great to me. The only thing that occurs to me, are the flake tests back to running? I don't see anything that should tickle it but I remember @grondo mentioning they were partially deactivated at some point.

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 2, 2018

@trws: that is a good point. I think pylint tests were running under the old travis config but have been turned off during the switch to Docker. I'll look into turning them back on. I believe they were turned off due to lots of errors from newer pylint versions.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 2, 2018

I believe they were turned off due to lots of errors from newer pylint versions.

Yeah, I had to disable pylint because it wasn't passing :-)

trws and others added some commits Aug 30, 2018

python: implement py3 compat
Co-authored-by: Stephen Herbein <herbein1@llnl.gov>
python/test: make python binding tests py3 compatible
  - Uses `six` package for most of the py2 and py3 support
  - Also uses `from __future__ import print_function`

Co-authored-by: Stephen Herbein <herbein1@llnl.gov>

@SteVwonder SteVwonder force-pushed the SteVwonder:python3-compatibility branch from 3871db9 to 1d3bc20 Oct 3, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 3, 2018

Pylint has been re-enabled, and the resulting errors fixed.

Bad first argument given to super class

The biggest change was switching super(self.__class__, self) to actually directly reference the class. self.__class__ works when the class is at the front of the MRO, but as soon as another class inherits from that class, an infinite recursion happens in the __init__. Most of the uses were in a class that was inside another class, so I doubt anyone would actually subclass them, but at least now pylint is happy.

no-else-return

For this error, in most cases, I obliged the linter, but in one particular case, I think the if/else is sufficiently large that the way it's currently written is easier to read/parse. So I overrode that case.

no-value-for-parameter

pylint really hates the ffi and lib modules being passed in as arguments to Wrapper.__init__. I don't see a better way to do it, so I just manually overrode each instance.

useless-object-inheritance

pylint3 doesn't like class WrapperBase(object). Disabled universally since this is required for py2 backwards compatibility.

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 3, 2018

Wow, nice work. I'm happy, and would say LGTM.

@SteVwonder SteVwonder force-pushed the SteVwonder:python3-compatibility branch 2 times, most recently from 52d0f22 to f5e05e5 Oct 4, 2018

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 4, 2018

It may be possible to fix some of this, but let me lay out the reasoning for the current pattern for context.

python file copying

This was part of the reason for breaking out the built extensions into _flux. The python searchpath only uses the first module that matches, this is what lua does as well actually, which meant that when everything was under flux.* all of the files had to be in the same directory which had to be named flux. The split may allow pythonpath manipulation to fix that, but I hadn't tested it.

so copying out of .libs

This is mainly because libtool deals with library search paths and library generation in general in the most frustrating way conceivable. Since nothing but libtool can actually read .la files, and the .so files are invalid in the build path without explicitly adding library paths to the environment, there's a non-trivial amount of working around that broken behavior. If we used anything else, that would just build the sos as valid in the build tree then re-path them on install, none of that would be necessary. If we can avoid it, I'm all for it, but that was the best way I could think of at the time to build with the same tooling as the rest of flux and still get the job done.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 4, 2018

Following up on a conversation @SteVwonder and I had yesterday about .libs. I had thought that its existence as an artifact of libtool was architecture dependent couldn't be portably assumed in build scripts. I remembered libtool on arm7l (32-bit) as the counter-example. However, I just checked that on a Pi and it works like other systems. I also verified 32-bit x86 works the same. Both were fairly recent debian-based.

If it isn't used on some platform or OS, I've lost track of which. Never mind?

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 4, 2018

I think you're right that there is a case where it doesn't create the .libs directory, but IIRC that's only on systems that don't support shared objects, which means our modules and python's couldn't be built anyway, so I wasn't worrying about it.

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 4, 2018

This was part of the reason for breaking out the built extensions into _flux. The split may allow pythonpath manipulation to fix that, but I hadn't tested it.

Embarrassingly enough, it was only after I removed the copying that the reason for the split finally "clicked" (thanks for the paving the way on that one). The split worked btw, there is no longer copying of source files over to the build tree (the exception being the __init__.py file in _flux).

Sidenote: the pymod macro that you (@trws) wrote is super handy! Thanks for that.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 4, 2018

I can't tell off-hand, but do any of the commits in here need to be squashed together? (Not trying to make useless work for you though!)

Also, a comment on commit messages: commit messages should be written in the imperative for consistency (e.g. check that pylint... not checks that pylint...), and the body should be wrapped at about 72 cols, or else various tools (and git long on my terminals 😉) will make weird line breaks.

You might only want to address the commit message comments if you have to squash and rebase anyway.

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 4, 2018

do any of the commits in here need to be squashed together

Yeah, I can squash some of the latter changes to the automake files. I did a squashing pass on the python changes earlier this week.

commit messages should be written in the imperative

Will do. Thanks!

the body should be wrapped at about 72 cols

Let me see if I can tweak my emacs config to enforce this automatically (I think the default is ~80)

trws and others added some commits Sep 28, 2018

python: break generated bindings out into a top-level module
  - moves generated shared libraries to a _flux directory
  - adds *.py and *.c gitignores to _flux directory

Done so that ultimately setuptools can be used for building the python
bindings.

Co-authored-by: Stephen Herbein <herbein1@llnl.gov>
python: track KVS and RPC C API changes
  - removes the user-passable flux handle to RPC which has changed from a
    flux_handle_t to a flux_future_t
  - removes a use of flux_rpc_multi
  - improves error checking in KVS API calls
t/python: move tests from /src/bindings/python/test to /t
Also includes minor cleanup of several of the tests
python: support utf-8 unicode strings
  - all input strings are checked to determine if they need to be
    encoded all
  - returned strings are decoded as 'utf-8', there are a few exceptions
    where the strings are decoded as 'ascii' (like getting the state of
    a job from the JSC)
pylint: enable pylint checking in travis
Disable unwanted and address relevant lint warnings.

Bad first argument given to super class:

The biggest change was switching `super(self.__class__, self)` to
actually directly reference the class.  `self.__class__` works when the
class is at the front of the MRO, but as soon as another class inherits
from that class, an infinite recursion happens in the `__init__`.  Most
of the uses were in a class that was inside another class, so I doubt
anyone would actually subclass them, but at least now pylint is happy.

no-else-return:

For this error, in most cases, I obliged the linter, but in one
particular case (src/bindings/python/flux/wrapper.py:224), I think the
if/else is sufficiently large that the way it's currently written is
easier to read/parse.  So I overrode that case.

no-value-for-parameter:

pylint really hates the `ffi` and `lib` modules being passed in as
arguments to `Wrapper.__init__`.  I don't see a better way to do it, so
I just manually overrode each instance.

useless-object-inheritance:

pylint3 doesn't like `class WrapperBase(object)`.  Disabled universally
since this is required for py2 backwards compatibility.
automake: minimize copying of python source files
add source tree to PYTHONPATH in AM_TEST_ENVIRONMENT to remove need to
copy the source py files into the build tree

add BUILT_SOURCES primary and use `nodist_` prefix to prevent
distribution of auto-generated c files

replace copying of python .so's out of .libs directory with symlinking
t/python: fix tests for systems with multiple python installs
 - use a LOG_DRIVER rather than direct execution
 - rename python tests to have .py extension rather than .t
 - tests now run under the same python interpreter that flux was
   configured with

This commit adds a thin wrapper around the tap-driver.sh to retain test
output granularity while still ensuring the python tests are executed
with the python interpreter that flux was configured with.

@SteVwonder SteVwonder force-pushed the SteVwonder:python3-compatibility branch from f5e05e5 to 33532ff Oct 4, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 4, 2018

Squashed a few commits and re-wrapped several commit message bodies to be < 72 characters.

This PR should be good to go now.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 4, 2018

Awesome!

Let me see if I can tweak my emacs config to enforce this automatically (I think the default is ~80)

Yeah, sorry, this isn't too important, but it keeps git log output looking nice with 80 char wide terminals, since the commit message is always indented at least 4 and up to 8. (probably I'm the only one this affects, if so feel free to tell me to be quiet!)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 4, 2018

Since @trws and @garlick approved this in writing and verbally, I'm merging.

@grondo grondo merged commit 1c86b38 into flux-framework:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 4, 2018

(probably I'm the only one this affects, if so feel free to tell me to be quiet!)

Not a problem at all; it is a pretty easily automated thing. Now that I have my emacs config setup, it shouldn't be a problem in the future.

Maybe we should consider adding a check for things like that and coding style to the CI so that maintainers don't have to bother asking for it to be addressed.

@SteVwonder SteVwonder deleted the SteVwonder:python3-compatibility branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.