Opened 10 years ago
Closed 9 years ago
#9631 closed defect (fixed)
Remerge #9501 after resolving NFS and/or doctest problems with @fork
Reported by: | mpatel | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | misc | Keywords: | |
Cc: | leif, jhpalmieri, kcrisman, malb, mvngu, SimonKing, was | Merged in: | sage-4.7.2.alpha2 |
Authors: | William Stein, Mitesh Patel | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Attachments (6)
Change History (26)
comment:1 Changed 10 years ago by
comment:2 follow-up: ↓ 3 Changed 10 years ago by
Ticket #10098 is about a similar [Errno 16] Device or resource busy
error.
comment:3 in reply to: ↑ 2 Changed 10 years ago by
Replying to mpatel:
Ticket #10098 is about a similar
[Errno 16] Device or resource busy
error.
See #10098's comment 9ff for a proposed possible solution.
comment:4 Changed 10 years ago by
- Milestone set to sage-4.6.1
- Status changed from new to needs_review
I've attached a tweaked version of William's patch v2 from #9501.
comment:5 Changed 9 years ago by
- Description modified (diff)
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
I've updated the patch for Sage-4.7.1, there were some rejects due to changed docstrings. I didn't touch any actual functionality.
The actual workings of the fork decorator look good. Positive review.
comment:6 Changed 9 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:7 Changed 9 years ago by
There are still a few typos in the docstrings.
Related (but shouldTM apply independently, haven't tested this yet): #11658
comment:8 follow-up: ↓ 9 Changed 9 years ago by
- Description modified (diff)
Hi Leif, I just attached trac_9631-fork_decorator.4.patch which polished the docstrings fixing some typos. If there are any remaining typos, please point them out explicitly.
comment:9 in reply to: ↑ 8 Changed 9 years ago by
Replying to was:
Hi Leif, I just attached trac_9631-fork_decorator.4.patch which polished the docstrings fixing some typos. If there are any remaining typos, please point them out explicitly.
-
trac_9631-fork_decorator.4.patch
old new 17 17 +++ b/sage/parallel/decorate.py 18 18 @@ -15,19 +15,17 @@ 19 19 r""" 20 Convert a to a pair (args, kwds)using some rules:20 Convert ``a`` to a pair ``(args, kwds)`` using some rules: 21 21 22 22 - * if already of that form, leave that way. 23 23 - * if a is a tuple make (a,{}) … … 44 44 @@ -53,9 +51,14 @@ 45 45 class Parallel: 46 46 r""" 47 Create paralleldecorated function.47 Create ``parallel``-decorated function. 48 48 - 49 49 """ 50 50 def __init__(self, p_iter = 'fork', ncpus=None, **kwds): … … 56 56 + """ 57 57 # The default p_iter is currently the reference implementation. 58 58 # This may change. 59 self.p_iter = None 59 self.p_iter = None # ??? = p_iter, which defaults to 'fork', not the sequ. ref. impl. 60 60 @@ -81,19 +84,16 @@ 61 61 62 62 def __call__(self, f): … … 67 67 - in possibly random order. Here x is replaced by its 68 68 + Create a function that wraps ``f`` and that when called with a 69 69 + list of inputs returns an iterator over pairs ``(x, f(x))`` in 70 + possibly random order. Here ``x`` is replaced by its71 normalized form (args, kwds) using normalize_inputs.70 + possibly random order. Here ``x`` is replaced by its 71 normalized form ``(args, kwds)`` using ``normalize_inputs()``. 72 72 73 73 INPUT: 74 74 - … … 102 102 + The parallel subprocesses will not have access to data 103 103 + created in pexpect interfaces. This behavior with respect to 104 104 + pexpect interfaces is very important to keep in mind when 105 + setting up certain computations. It's the one big limitation105 + setting up certain computations. It's the one big limitation 106 106 + of this decorator. 107 107 + 108 108 INPUT: … … 114 114 + - ``fork`` -- (default) use a new forked process for each input 115 115 + - ``multiprocessing`` -- use multiprocessing library 116 116 + - ``reference`` -- use a fake serial reference implementation 117 - ``ncpus`` -- integer, number of cpus118 - ``timeout`` -- number of seconds until task is killed (only supported by 'fork')117 - ``ncpus`` -- integer, maximal number of subprocesses to use at the same time 118 - ``timeout`` -- number of seconds until a subprocess is killed (only supported by ``fork``; zero means not at all) 119 119 120 120 + .. warning:: 121 121 + 122 + If you use anything but 'fork'above, then a whole new122 + If you use anything but ``fork`` above, then a whole new 123 123 + subprocess is spawned, so none of your local state (variables, 124 124 + certain functions, etc.) is available. 125 125 + 126 126 EXAMPLES: 127 127 128 We create a simple decoration for a simple function. The number128 We create a simple decoration for a simple function. The number 129 129 @@ -148,7 +166,6 @@ 130 130 sage: @parallel(2) 131 131 ... def f(n): return n*n 132 132 133 133 - 134 We create a decorator that uses 3processes, and times out134 We create a decorator that uses three subprocesses, and times out 135 135 individual processes after 10 seconds:: 136 136 137 137 @@ -174,3 +191,152 @@ … … 144 144 + 145 145 +################################################################### 146 146 +# The @fork decorator -- evaluate a function with no side effects 147 +# in memory, so the only side effects are on disk.147 +# in memory, so the only side effects (if any) are on disk. 148 148 +# 149 149 +# We have both a function and a class below, so that the decorator 150 150 +# can be used with or without options: … … 158 158 + 159 159 +class Fork: 160 160 + """ 161 + A fork decorator class.161 + A ``fork`` decorator class. 162 162 + """ 163 163 + def __init__(self, timeout=0, verbose=False): 164 164 + """ 165 165 + INPUT: 166 + - ``timeout`` -- (default: 0) kill s subrocess afterthis many167 + seconds , or if timeout=0, do not kill the subprocess.166 + - ``timeout`` -- (default: 0) kill each subprocess after it has run this many 167 + seconds (wall time), or if ``timeout`` is zero, do not kill any subprocesses. 168 168 + - ``verbose`` -- (default: ``False``) whether to print 169 169 + anything about what the decorator does (e.g., killing 170 170 + subprocesses) … … 182 182 + def __call__(self, f): 183 183 + """ 184 184 + INPUT: 185 185 186 + - ``f`` -- a function 186 187 + 187 188 + OUTPUT: 189 188 190 + - A decorated function. 189 191 + 190 192 + EXAMPLES:: … … 206 208 + """ 207 209 + Decorate a function so that when called it runs in a forked 208 210 + subprocess. This means that it won't have any in-memory 209 + side -effects on the parent Sage process. The pexpect interfaces211 + side effects on the parent Sage process. The pexpect interfaces 210 212 + are all reset. 211 213 + 212 214 + INPUT: 213 215 + - ``f`` -- a function 214 + - ``timeout`` -- (default: 0) if positive, kill s subrocess after215 + this many seconds 216 + - ``timeout`` -- (default: 0) if positive, kill subprocess after 217 + this many seconds (wall time) 216 218 + - ``verbose`` -- (default: ``False``) whether to print anything 217 + about what the decorator does (e.g., killing subprocesses)219 + about what the decorator does (e.g., killing the subprocess) 218 220 + 219 221 + .. warning:: 220 222 + 221 + The forked subprocess eswill not have access to data created223 + The forked subprocess will not have access to data created 222 224 + in pexpect interfaces. This behavior with respect to pexpect 223 225 + interfaces is very important to keep in mind when setting up 224 + certain computations. It's the one big limitation of this226 + certain computations. It's the one big limitation of this 225 227 + decorator. 226 228 + 227 229 + EXAMPLES: 228 230 + 229 + We create a function and run it with the forkdecorator. Note231 + We create a function and run it with the ``fork`` decorator. Note 230 232 + that it does not have a side effect. Despite trying to change 231 + the global variable "a" below in g, the variable adoes not get232 + changed .::233 + the global variable ``a`` below in ``g``, the variable ``a`` does not get 234 + changed:: 233 235 + 234 236 + sage: a = 5 235 237 + sage: @fork … … 242 244 + sage: a 243 245 + 5 244 246 + 245 + We use fork to make sure that the function terminates after 1247 + We use ``fork`` to make sure that the function terminates after one 246 248 + second, no matter what:: 247 249 + 248 250 + sage: @fork(timeout=1, verbose=True) … … 253 255 + Killing subprocess ... with input ((10000000,), {'m': 5}) which took too long 254 256 + 'NO DATA (timed out)' 255 257 + 256 + We illustrate that pexpect interface state is not affected by258 + We illustrate that the state of the pexpect interface is not altered by 257 259 + forked functions (they get their own new pexpect interfaces!):: 258 260 + 259 261 + sage: gp.eval('a = 5') … … 305 307 - - ``timeout`` -- (float) time in seconds until a subprocess is automatically killed 306 308 - - ``verbose`` -- whether to print anything about what the iterator does (e.g., killing subprocesses) 307 309 + - ``ncpus`` -- the maximal number of simultaneous 308 + processes to spawn309 + - ``timeout`` -- (float, default: 0) time in seconds until310 + subprocesses to spawn 311 + - ``timeout`` -- (float, default: 0) wall time in seconds until 310 312 + a subprocess is automatically killed 311 313 + - ``verbose`` -- (default: False) whether to print 312 314 + anything about what the iterator does (e.g., killing 313 315 + subprocesses) 314 316 + - ``reset_interfaces`` -- (default: True) whether to reset 315 + all expect interfaces317 + all pexpect interfaces 316 318 317 319 EXAMPLES:: 318 320 … … 326 328 """ 327 329 @@ -206,7 +213,8 @@ 328 330 329 # The expect interfaces (and objects defined in them) are331 # The pexpect interfaces (and objects defined in them) are 330 332 # not valid. 331 333 - sage.interfaces.quit.invalidate_all() 332 334 + if self.reset_interfaces:
Explicit, but perhaps a bit inconvenient. Didn't apply it, only looked at the patch.
comment:10 Changed 9 years ago by
P.S.: The wording w.r.t. ncpus
and timeout
could be unified.
comment:11 Changed 9 years ago by
Since you changed the code, you should post a patch, since it seems silly for me to just manually copy your changes in...
comment:12 follow-up: ↓ 13 Changed 9 years ago by
I see -- you literally patched the patch? I don't know how to get your patch of the patch off of trac. I can view it, but can't download it...?
comment:13 in reply to: ↑ 12 Changed 9 years ago by
Replying to was:
I see -- you literally patched the patch? I don't know how to get your patch of the patch off of trac. I can view it, but can't download it...?
Yep, sorry. (I actually edited your patch and then made a diff just for displaying.)
In principle one can extract inline patches from mail notifications (or "reply" to the comment just to copy parts), but since I also changed context lines, the patched patch wouldn't apply anyway.
I was just going to upload a v5 (no code changes btw., just in comments), but somehow managed to totally mess up the patch (and apparently also the files themselves) such that I now have two mixed versions with both having some of your and some of my changes... 8/
Trying to fix this, then I'll upload a "real" v5 patch; I made more changes than in the comment above.
Changed 9 years ago by
Diff between v4 (William's) and v5 (Leif's) patch. For reference / review only.
comment:14 Changed 9 years ago by
Ok, attached a v5 patch, and a diff between William's v4 and my version.
Take a look at it, haven't updated the description yet.
Hope I didn't miss something, as I had to redo almost all from scratch, because some very weird things must have happened. Not only did the editor confuse files, presumably due to renaming and symbolic links, but also my first committed and exported version completely vanished from the Mercurial repository, which must be some weird bug related to cloning and rebuilding a branch.
comment:15 follow-up: ↓ 16 Changed 9 years ago by
- Status changed from positive_review to needs_work
Looks good. If you can add a description then we'll be good to go.
comment:16 in reply to: ↑ 15 Changed 9 years ago by
Replying to vbraun:
Looks good. If you can add a description then we'll be good to go.
? I just meant I haven't updated the ticket's description to refer to the new v5 patch.
comment:17 Changed 9 years ago by
- Status changed from needs_work to needs_info
comment:18 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_info to needs_review
Sorry, I didn't notice that the patch does already include some description; since it starts with a hash-mark its somewhat indistinguishable from the automatic mercurial comments. But should be good enough.
comment:19 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:20 Changed 9 years ago by
- Merged in set to sage-4.7.2.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
After the recent changes on the Sage cluster, I still see failures on sage.math with 4.6.alpha2 + #9501's v2 patch:
I believe the
Unhandled SIGSEGV
is expected.