search

cxcsds / ciao-contrib

Extra scripts and code to enhance the capabilities of CIAO.
GNU General Public License v3.0
8 stars 6 forks source link

runtool: Set `ASCDS_*` in call to `subprocess.POpen`? #317

Open hamogu opened 4 years ago

hamogu commented 4 years ago

I'm running different instances of wavdetect and run into trouble, because names in the temporary directory clash. That's a known limitation (from the bug and limitations page) with the recommended workaround of setting ASCDS_WORK_PATH before invoking wavdetect. However, it seems that that's not sufficient. Clashes can also happen in the tempdir when input file names are the same (I call the image "image" in every directory). This would require changing the ASCDS_TMP variable as well.

So, I started digging into the implementation and now I'm wondering, if, instead of or in addition to what the CIAOTool classes currently do, it would make sense to call subprocess.Popen with the env parameter and set the appropriate ASCDS_* and PFILES environment variables there so that the CIAO tool sees the correct environment. That might also alleviate the need to distinguish between CIAOToolDirect and CIAOToolParFile. I don't make this a PR now, because I don't fully grasp the logic behind all the pfiles manipulation here and because I have more urgent things to do before the next marx release.

This issue is here so that @DougBurke can tell me that it's a waste of time to think about that any more, or, if that's not the case, so that he can prod me in 2020 and can remind me to produce a proof-of-concept PR to see if it's worth going that way.

DougBurke commented 4 years ago

Just to note that the main reaon for CIAOToolParFile and CIAOToolDirect is that one allows you to use the syntax tool @@foo.par and one doesn't - i.e. is not related to environment files. Not that that is relevant to the main thesis of this issue.

DougBurke commented 4 years ago

I think it would be worth documenting the current environment shenanigans changes that runtool makes, and why they are (or were) needed. The current behavior has grown "organically", so reviewing it might make sense.

For instance, should the CIAOTool* classes always create their own temporary directory for PFILES (for one off, simple use cases), or is the current approach of having the user be explicit about it with the context managers make sense.

Maybe it would make sense to add a context manager that sets ASCDS_TMP, ASCDS_WORK_PATH, ... to a temporary directory. This is cleaner/more explicit, but how's the UX? One concern I have about the runtool functions automatically doing this is if there's a case when you would want to set these directories to certain values (e.g. directories being used by other processes that you want to share files with).