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: Several fixes for the bindings #794

Merged
merged 3 commits into from Sep 8, 2016

Conversation

Projects
None yet
6 participants
@SteVwonder
Copy link
Member

SteVwonder commented Sep 7, 2016

Several python binding fixes wrapped into one PR:

  • Add constants for FDWatcher
  • Switch kvs.py to use the latest KVS interface (remove use of json-c)
  • Add interface to JSC

Fixes #793

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Sep 7, 2016

Hold off on merging this until we figure out the EAGAIN issue in #793. Sorry, was a little too quick to push.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 7, 2016

Coverage Status

Coverage decreased (-0.008%) to 74.974% when pulling 07184a6 on SteVwonder:python-fixes into d1da47b on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 7, 2016

Current coverage is 74.65% (diff: 100%)

Merging #794 into master will decrease coverage by <.01%

@@             master       #794   diff @@
==========================================
  Files           145        145          
  Lines         24960      24960          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          18636      18634     -2   
- Misses         6324       6326     +2   
  Partials          0          0          

Powered by Codecov. Last update d1da47b...07184a6

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 7, 2016

This looks good to me. I may pop in a wrapper to abstract some of this stuff and propagate it to other parts of the bindings, but I'd say LGTM. Any concerns @garlick, @grondo?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 7, 2016

No concerns here!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 7, 2016

It's probably pretty safe, but is there a cleaner way to import those constants FLUX_POLLIN, FLUX_POLLOUT, FLUX_POLLERR? As a general rule duplicating constants in the bindings is a potential source of future unhappiness.

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Sep 7, 2016

@garlick: that is a great point. I had to pull in quite a few constants. I don't think I can do anything about the preprocessor macros/defines (e.g. JSC_STATE_PAIR), but something should be able to be done for the enums (e.g. J_SUBMITTED, FLUX_POLLIN).

@trws: do you have any experience with this? I was unsuccessful yesterday in googling how to handle enums with py-cffi. I noticed that the enums are present in the generated .c files, but I am unsure how to access them. Taking FLUX_POLLIN for example, it appears in _core.c in the _cffi_enums struct, but it is unclear how accessing that works.

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 7, 2016

I'll look into it. There wasn't (to my extreme discomfort) a good way to do it when I wrote them, but the ffi interface may have grown one since.


Sent from Boxer | http://getboxer.comhttp://bxr.io/PBI3C

On September 7, 2016 at 10:35:17 AM PDT, Jim Garlick notifications@github.com wrote:

It's probably pretty safe, but is there a cleaner way to import those constants FLUX_POLLIN, FLUX_POLLOUT, FLUX_POLLERR? As a general rule duplicating constants in the bindings is a potential source of future unhappiness.

You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com//pull/794#issuecomment-245357343, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAoStc-O0RQkEq8l4Ku1AAd-3UIFFPdsks5qnvU6gaJpZM4J2-PA.

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 7, 2016

There is a way, but it involved generating new externed C values, and there were issues with parsing the constant expressions in some of our enums that broke that stuff. I'll take a look at it this afternoon, I have an idea that might work, but I'm not sure if it will work with the auto generation stuff.


Sent from Boxer | http://getboxer.comhttp://bxr.io/PBI3C

On September 7, 2016 at 11:26:09 AM PDT, Stephen Herbein notifications@github.com wrote:

@garlickhttps://github.com/garlick: that is a great point. I had to pull in quite a few constants. I don't think I can do anything about the preprocessor macros/defines (e.g. JSC_STATE_PAIR), but something should be able to be done for the enums (e.g. J_SUBMITTED, FLUX_POLLIN).

@trwshttps://github.com/trws: do you have any experience with this? I was unsuccessful yesterday in googling how to handle enums with py-cffi. I noticed that the enums are present in the generated .c files, but I am unsure how to access them. Taking FLUX_POLLIN for example, it appears in _core.c in the _cffi_enums struct, but it is unclear how accessing that works.

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/794#issuecomment-245373021, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAoStQQuR_A0QwISdyMl2inEiVlp3KfOks5qnwG4gaJpZM4J2-PA.

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 7, 2016

Wow, so, if we're willing to bump the revision of cffi a few versions, I can get all of the enums (except maybe one nasty one) to be extracted automatically, and also clean up and speed up the callbacks so they don't require runtime generation of executable code regions on the heap. I'll put up a PR with the enum part at least, and probably a separate one that has the callback rework.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 7, 2016

if we're willing to bump the revision of cffi a few versions,

Here are the versions we have around

  • TOSS 3: python-cffi-1.5.0-2.ch6 (locally rolled for root)
  • TOSS 2: python2.7-cffi-1.1.2-1.ch5.3 (locally rolled in /opt)
  • Ubuntu 16.04.1 LTS: 1.5.2-1ubuntu1 (from canonical) my desktop - maybe others don't care

orchestrating an update may be tricky if you need > those versions.

@trws

This comment has been minimized.

Copy link
Member

trws commented Sep 7, 2016

Actually, that should be good at least as far as toss3 is concerned. I
think the most recent thing I was looking at was added in 1.5, the enum
stuff might be good on both, but I'll have to test it.

On 7 Sep 2016, at 13:24, Jim Garlick wrote:

if we're willing to bump the revision of cffi a few versions,

Here are the versions we have around

  • TOSS 3: python-cffi-1.5.0-2.ch6 (locally rolled for root)
  • TOSS 2: python2.7-cffi-1.1.2-1.ch5.3 (locally rolled in /opt)
  • Ubuntu 16.04.1 LTS: 1.5.2-1ubuntu1 (from canonical) my desktop -
    maybe others don't care

orchestrating an update may be tricky if you need > those versions.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#794 (comment)

@trws trws merged commit 3fd9da0 into flux-framework:master Sep 8, 2016

4 checks passed

codecov/patch Coverage not affected when comparing d1da47b...07184a6
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +25.33% compared to d1da47b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.008%) to 74.974%
Details

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

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.