python: split flux.job module into multiple files #3162
Conversation
grondo
force-pushed
the
python-split-job-module
branch
3 times, most recently
from
eafe664 to
f0a67dc
Compare
|
Ok, sorry for the noise. It took me a few tries to get the Makefile right. The recursive |
|
Restarted two builders that hit an error in the initial clone from github. |
|
I had not encountered |
|
Ok, I think this is largely working. No code changes, just reorganization, is probably a good place to stop for this PR, so probably one of the resident Python experts should review this. (@SteVwonder and/or @trws) I'm a little worried this change could have an adverse performance impact. |
There was a problem hiding this comment.
This looks good, and should have pretty small performance impact. I'd say zero, but packaged as separate files it will have a small impact at load time unless the directory is zipped as part of the install process, then it would be basically zero.
My only thought is that the wrapper file, or the RAW in it might be good to be renamed as _RAW or _wrapper.py or both. I realize I started the whole RAW thing 🤦 but making it something that doesn't get loaded from a * import and at least conventionally signals it's internal would be nice.
The actual re-homing looks really nice btw. 👍
|
Awesome! This looks great to me! In terms of performance, there will be an added cost since the python interpreter will need to find and import 9 more files than it used to, but my guess is that will be completely in the noise except in pathological cases (i.e., having a Spack environment loaded and running across 1000s of nodes). For all of the other imports that are now duplicated across the new files (e.g., For the pathological cases, I suspect Spindle will be more than enough to handle those cases. |
Great suggestion! I was wondering exactly how I should do that. I'm fine renaming both. |
Yeah, this should be right. One thing I mentioned as an aside above but found really, really helped when doing some of the initial mummi stuff was setting up the big directory of modules in a zip file: import modules from zip archives. The interpreter can treat a zip as a directory, and it caches the whole file along with its internal directory listing on initial load then does lookups in it rather than loading all from disk. So if it became an issue a bit of packaging rework could probably squash it without too much trouble. |
grondo
force-pushed
the
python-split-job-module
branch
from
f0a67dc to
43c4563
Compare
|
Thanks @trws and @SteVwonder! I've force-pushed a change to rename |
grondo
changed the title
grondo
force-pushed
the
python-split-job-module
branch
from
43c4563 to
5f75ad3
Compare
|
Hey,
I am going to assume that I was tagged on accident? Unless you want me to review it, in which case I am sure I could give it my best :P
Cheers,
Bert
… On Aug 25, 2020, at 07:54, Mark Grondona ***@***.***> wrote:
Ok, I think this is largely working. No code changes, just reorganization, is probably a good place to stop for this PR, so probably one of the resident Python experts should review this. ***@***.*** <https://github.com/SteVwonder> and/or @bertjwregeer <https://github.com/bertjwregeer>
I'm a little worried this change could have an adverse performance impact.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3162 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE6RUWZBMPLM4ICKWQCY2LSCPGCPANCNFSM4QJ7MFWA>.
|
|
Hahaha, yeah that was a strange GitHub autocorrect snafu
On Tue, Aug 25, 2020, 6:29 PM Bert JW Regeer <notifications@github.com>
wrote:
… Hey,
I am going to assume that I was tagged on accident? Unless you want me to
review it, in which case I am sure I could give it my best :P
Cheers,
Bert
> On Aug 25, 2020, at 07:54, Mark Grondona ***@***.***>
wrote:
>
>
> Ok, I think this is largely working. No code changes, just
reorganization, is probably a good place to stop for this PR, so probably
one of the resident Python experts should review this. ***@***.*** <
https://github.com/SteVwonder> and/or @bertjwregeer <
https://github.com/bertjwregeer>
> I'm a little worried this change could have an adverse performance
impact.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <
#3162 (comment)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAE6RUWZBMPLM4ICKWQCY2LSCPGCPANCNFSM4QJ7MFWA
>.
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3162 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFVEUQ6OO2ELYSD7TRO3BTSCRQN3ANCNFSM4QJ7MFWA>
.
|
|
It looks like this one was approved so I'm setting MWP since I have other work requiring this PR. |
|
Actually this won't pass travis until #3169 is merged. |
Combine the Makefile.am files in src/bindings/python/flux and src/bindings/python/flux/core into a single Makefile.am. Use nobase_ prefix so that all Python source files can be listed in the same rule. The special `fluxpycoredir` automake variable is no longer needed, so it can be dropped. This should make the bindings Makefiles a bit easier to maintain by reducing duplication and the number of files to edit when new Python source files are added.
grondo
force-pushed
the
python-split-job-module
branch
from
5f75ad3 to
548ded9
Compare
Problem: The Python 'flux.job' module is large (>1200 lines) and thus getting difficult to read and/or modify. This module will only get larger since there will certainly be more classes and methods related to Flux jobs added to the Python API. Split `src/bindings/python/flux/job.py` into multiple files, while attempting to keep related code together without making any one component too large. For backwards compatibility, recreate the previous `flux.job` namespace in the __init__.py in the new job directory.
grondo
force-pushed
the
python-split-job-module
branch
from
548ded9 to
6cc9dda
Compare
Codecov Report
@@ Coverage Diff @@
## master #3162 +/- ##
==========================================
+ Coverage 81.21% 81.24% +0.03%
==========================================
Files 286 294 +8
Lines 44670 44366 -304
==========================================
- Hits 36277 36045 -232
+ Misses 8393 8321 -72
|
At this point, this work is a request for comments. I don't know if there are drawbacks to splitting a single python module into multiple files -- will this slow down
import flux.jobsignificantly?This does prepare the way for an easier addition of the
flux.job.JobInfoclass (and related functions) to the bindings though.