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

python: add job & mrpc bindings #1757

Merged
merged 16 commits into from Nov 5, 2018

Conversation

Projects
None yet
3 participants
@SteVwonder
Copy link
Member

SteVwonder commented Oct 24, 2018

The bulk of this PR is the addition of bindings and tests for job.h and mrpc.h.

A few minor changes also included:

  • adds support for flux testing personalities (e.g. job or full)
    • required for testing the job bindings
  • improves RPC errors by automatically calling flux_future_error_string on failure
  • fixes #1738 by using ffi.buffer rather than ffi.unpack
  • fixes some typos and copy-paste errors in mrpc.h and t/python
  • pycotap now will not throw its own exception when an exception occurs within a test's setUpClass
  • closes #1741 by launching the munge daemon within the test container
  • closes #1781 by replacing calls to flux.core.Flux with flux.Flux, so that flux.core no longer needs to import flux.core.handle.Flux
  • related to #1649 (mrpc bindings)

@SteVwonder SteVwonder requested a review from trws Oct 24, 2018

@trws
Copy link
Member

trws left a comment

This all looks pretty good to me. There's one or two minor nits in the comments, but short of some spacing stuff and making travis happy I'm good.

Show resolved Hide resolved src/bindings/python/flux/Makefile.am Outdated
Show resolved Hide resolved src/bindings/python/flux/job.py
Show resolved Hide resolved src/bindings/python/flux/job.py
@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 24, 2018

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch from 465b9e7 to 9c2c2a0 Oct 30, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 30, 2018

@trws: if you are ok with the changes made in the latest 3 commits and Travis goes green, then I'll squash those FIXUP commits and rebase onto master.

EDIT: I have to fix #1741 and then hopefully Travis will be green (there still could be either a py3 or distcheck issue lurking).

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch from 223c849 to 3491e92 Oct 30, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 30, 2018

Got the all green from Travis on my fork. Just rebased onto upstream/master. Hopefully Travis comes back green again.

In the meantime, this is ready for your re-review @trws.

@grondo, you might be interested in reviewing the two docker: commits.

EDIT: nope, I borked something during what was a relatively painless rebase. 😞

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch 3 times, most recently from 7028cc1 to 79e1f71 Oct 30, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 31, 2018

Ok. For real this time: rebased onto master and Travis is green.

Let me know if the scope creep on this PR is too great, and I can break it up into two separate PRs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 31, 2018

Codecov Report

Merging #1757 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1757      +/-   ##
==========================================
+ Coverage   79.59%   79.63%   +0.03%     
==========================================
  Files         186      186              
  Lines       34563    34563              
==========================================
+ Hits        27511    27524      +13     
+ Misses       7052     7039      -13
Impacted Files Coverage Δ
src/modules/pymod/py_mod.c 65.06% <100%> (ø) ⬆️
src/cmd/flux-module.c 85.28% <0%> (-0.61%) ⬇️
src/common/libflux/message.c 81.02% <0%> (ø) ⬆️
src/modules/kvs-watch/kvs-watch.c 75.96% <0%> (+0.82%) ⬆️
src/modules/barrier/barrier.c 78.62% <0%> (+2.06%) ⬆️
src/common/libjob/job.c 60.46% <0%> (+20.93%) ⬆️
@trws

trws approved these changes Nov 1, 2018

Copy link
Member

trws left a comment

This looks like a good set of changes to me. There are some questions elsewhere in the review, but nothing that strikes me as blocking. Thanks @SteVwonder!

Show resolved Hide resolved src/bindings/python/flux/core/handle.py Outdated
Show resolved Hide resolved src/bindings/python/flux/util.py
Show resolved Hide resolved src/test/travis_run.sh Outdated

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch 2 times, most recently from 23940a7 to 08425de Nov 2, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Nov 2, 2018

Rebased on master so that Travis runs. Still need to squash incremental development.

docker: fix OS check in travis Dockerfile
- change OS -> BASE_IMAGE to avoid collision when exporting as env var
  - will be useful later on for other script that want to know what
    image is being used
- change 'ubuntu*' -> 'bionic*'
- add catch-all ('*') case to log to stderr when base_image is unknown

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch from 08425de to b5ca6b9 Nov 2, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Nov 2, 2018

Incremental development has been squashed

SteVwonder added some commits Oct 11, 2018

python/security: switch from ffi unpack to buffer for sign_unwrap
unpack assumes the data is a string, while buffer makes no assumptions
of the data's underlying type
python: refactor Message,Job,RPC,MRPC to reduce duplicate code
Pylint was flagging large portions of these files as duplicated. Pulled
common functionality out into functions that can be used across all
three.

Add a decorator for functions/methods that call flux_future_get and can
benefit from calls to flux_future_error_string when errors occur.

Fix several other minor pylint warnings and silence the 'duplicate-code'
warning on the `get` methods and constructors of RPC and MRPC.

Once the circular imports in RPC and MRPC are fixed, a common object
ancestor may make sense for these two classes

Ensure the conversion of unicode rpc topic strings to binary strings

SteVwonder added some commits Oct 30, 2018

python/pycotap: set a default message stream
Currently, if an exception occurs within the setUpClass method of a
test, then `self.message` is None and an exception occurs within
pycotap, obscuring the original exception from the test. Set a default
message stream to avoid exceptions from within pycotap, preserving the
original exception from the test setup.
python: fix circular imports
Importing flux.rpc, flux.mrpc, and flux.util would result in a circular
import. Replace calls to `flux.core.Flux` with `flux.Flux`.

@SteVwonder SteVwonder force-pushed the SteVwonder:py-bindings-job-mrpc branch from b5ca6b9 to 72638ec Nov 2, 2018

@trws

This comment has been minimized.

Copy link
Member

trws commented Nov 3, 2018

Ready to merge @SteVwonder?

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Nov 3, 2018

👍 Yep. Ready for merging.

@trws trws merged commit 7394541 into flux-framework:master Nov 5, 2018

3 checks passed

codecov/patch 100% of diff hit (target 79.59%)
Details
codecov/project 79.63% (+0.03%) compared to af34809
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trws

This comment has been minimized.

Copy link
Member

trws commented Nov 5, 2018

Thanks again @SteVwonder!

@SteVwonder SteVwonder deleted the SteVwonder:py-bindings-job-mrpc 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.