Conversation
|
Can run in parallel with e.g. (for docker image name pv): |
chrisroat
left a comment
chrisroat
left a comment
There was a problem hiding this comment.
Thanks for the updates! I have some questions below about some of the changes.
| --shm-size=1g \ | ||
| --env=TZ=America/Los_Angeles \ | ||
| ${1} | ||
| ## Support running in parallel: |
There was a problem hiding this comment.
I'm curious why you choose to do this in a shell script, compared to launching multiple jobs (or an array job) via slurm?
The reason I ask is because this seems to be adding some complexity around xvfb and locking (which looks tricky and is just one more thing that could break....), whereas parallel launching via slurm is straightforward. Also, it gets tricky to manage ending gracefully and/or doing retries in case where one or more of the parallel threads fails for some reason.
There was a problem hiding this comment.
I've been running this script on my local computer to avoid filesystem slowness, so this is nice for parallelization, but perhaps better to be in new script..?
| fs_param = 1. / (mdata['period'] * z_planes) | ||
|
|
||
| # Load suite2p only right before use, as it has a long load time. | ||
| import suite2p |
There was a problem hiding this comment.
This loads in all of suite2p (which includes the gui stuff and the detection modules). It's not fatal, but I've found suite2p to be heavy weight. So I had just imported the subset of libraries (run_s2p) that we use below.
There was a problem hiding this comment.
latest version of suite2p refactored a bit, e.g. need suite2p.default_ops() but maybe not fully necessary
| # processing finishing. The following variables relate to the timing of that polling | ||
| # process. | ||
| RIP_TOTAL_WAIT_SECS = 3600 # Total time to wait for ripping before killing it. | ||
| RIP_TOTAL_WAIT_SECS = 10 # Total time to wait for ripping before killing it. |
There was a problem hiding this comment.
Doesn't this kill the ripper after one pass through the while loop at line 116? That seems too soon, no?
There was a problem hiding this comment.
oops, I had changed this to be a really large number as a hack to fix voltagerecording output and meant to revert back to previous
| version = root.attrib['version'] | ||
|
|
||
| # Prairie View versions are given in the form A.B.C.D. | ||
| # TODO: allow match when minor version mismatches, eg fall back to `version = '5.5.1.1'` |
There was a problem hiding this comment.
Currently, this uses the major+minor to determine which ripper version to use. It seems, at least anecdotally, that the minor version is important.
What behavior are you expecting on a minor version mismatch?
One improvement I see is that we should throw an error right here if the ripper version is not in the repo.
There was a problem hiding this comment.
yes you're right i think minor version is sometimes (usually) important...annoying...
There was a problem hiding this comment.
Then this TODO probably isn't necessary. We currently handle 5.4 vs 5.5, and I'd like the code to fail loudly (not silently try something we haven't tested) if it detects anything else.
2 participants
Partial resolution to #7