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: misc fixes, refactors, and additions #2218

Merged
merged 7 commits into from Jul 9, 2019

Conversation

@SteVwonder
Copy link
Member

commented Jul 8, 2019

Main changes are fixing streaming RPCs, fixing job.get_id(), and adding a CLIMain decorator for uniform exception handling by CLI tools written in Python.

Minor changes include adding some methods to the Flux handle class, improve error messages, and extending the check-format/format scripts to be usable as git commit hooks.

Fixes #2155

SteVwonder added 5 commits May 14, 2019
If no arguments are provided, it will traverse the entire source tree
looking for python files to format.  If arguments are provided, only
those files are considered for checking.  The script will filter out any
non-python files from the arguments list before passing them to `black`.

Useful for git pre-commit scripts, which can produce a list of files
that changed in a given commit.  This results in a much lighterweight
operation than traversing the entire source tree and opening every file.
Generally, we want to keep user's terminals free of stacktraces from
Python (unless they have verbose logging on).  By moving the common
top-level exception handling to a decorator, hopefully all our CLI tools
will avoid dumping stacktraces unnecessarily, have consistent output,
and avoid duplicate code.
adds event_subscribe to improve auto-completion suggestions

adds respond method to provide automatic conversion of the message and
payload arguments as well as improve auto-completion suggestions
@SteVwonder SteVwonder requested review from trws and grondo Jul 8, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

This all looks good to me, though I'm not confident in reviewing a lot of the python code.

I do think 3995f32 could use a commit body which describes the problem with streaming RPC then callbacks and how it is resolved by the commit (perhaps some comments in the implementation as well, though that could be due to my python inexperience)

It was a good idea to abstract and standardize some of the front-end command setup into a utility module. I wonder if that should eventually be wrapped up into an exported module for use in other flux framework projects? (not needed for now though)

Copy link
Member

left a comment

Good stuff @SteVwonder.

src/bindings/python/flux/future.py Show resolved Hide resolved
src/bindings/python/flux/util.py Show resolved Hide resolved
SteVwonder added 2 commits May 12, 2019
If the same future has its callback executed (e.g., in the case of a
streaming RPC), an error occurs. After the first execution of the
callback, the bindings remove the python-side reference to the data
handle. So by the time that the second call happens, the data handle has
already been garbage collected.

Add a reference count dictionary to keep futures with a pending `then` callback
alive, even if there are no remaining references to the future in the user's
program scope. When a callback is first set on the future, add an entry to the
dictionary with the future itself as the key and a value of 1. Whenever a
future's callback is run, decrement the count in the dictionary for that
future, and whenever reset is called on a future, increment the counter. If
the counter hits 0, delete the reference to the future from the dictionary.
Wrap the return of `submit_async` in a python Future object rather than
return an opaque, c future pointer so that users can leverage all python
Future methods.

Add a `wait_for` to ensure that the future is fulfilled before getting
the id.  Otherwise, an error occurs if the submission future is not
fulfilled when `submit_get_id` is called.
@SteVwonder SteVwonder force-pushed the SteVwonder:python-grabbag branch from 84007cb to a13229d Jul 8, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

I do think 3995f32 could use a commit body which describes the problem with streaming RPC then callbacks and how it is resolved by the commit

👍 Force-pushed a new version of the streaming RPC commit that includes a message body in the commit as well as comments within the code about the semantics of the reference count dictionary.

I wonder if that should eventually be wrapped up into an exported module for use in other flux framework projects?

Yeah. That's a good point. I think we have that functionality now. All of the python bindings are exported/installed, so flux-sched and flux-capacitor should be able to use import flux.util to pull in those functions.

@codecov-io

This comment has been minimized.

Copy link

commented Jul 8, 2019

Codecov Report

Merging #2218 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2218      +/-   ##
=========================================
+ Coverage   80.69%   80.7%   +<.01%     
=========================================
  Files         202     202              
  Lines       32259   32259              
=========================================
+ Hits        26032   26034       +2     
+ Misses       6227    6225       -2
Impacted Files Coverage Δ
src/common/libflux/message.c 80.62% <0%> (-0.26%) ⬇️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/modules/barrier/barrier.c 80.53% <0%> (+2.01%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

All of the python bindings are exported/installed, so flux-sched and flux-capacitor should be able to use import flux.util to pull in those functions.

Ah, great! Sorry I didn't notice that.

@grondo
grondo approved these changes Jul 8, 2019
Copy link
Contributor

left a comment

Thanks @SteVwonder, LGTM!

@grondo grondo merged commit 60f2ffa into flux-framework:master Jul 9, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing a2a44e1...a13229d
Details
codecov/project 80.7% (+<.01%) compared to a2a44e1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.