search

pypest / pyemu

python modules for model-independent uncertainty analyses, data-worth analyses, and interfacing with PEST(++)
BSD 3-Clause "New" or "Revised" License
176 stars 98 forks source link

Idea to move from `os.system` to `sp.Popen` in os_utils.run :) #532

Closed p-ortega closed 1 month ago

p-ortega commented 3 months ago

Hey Brioch, Just found this behavior and I reckon it could add to the argument to move os system to subprocess.

Lemme know what do you think

Cheers

Description: platform: Windows

Running the test from util_tests.py::test_run [pyemu.os_utils.run(echo test)] returns 'exe test' instead of 'test' without raising an exception. This is coming from the function adding .exe to the string if not defined.

I reckon this is where subprocess could come in handy. The same function with subprocess.Popen returns 'file not found'.

jtwhite79 commented 2 months ago

I think that test is doing what we want it to - its checking on PC that echo is a command and that pyemu.os_utils.run() is appending ".exe" to echo and then the command is successful (which is this case, results in "exe test" being echo'd to stdout). The other obvs bogus run ("junk") does return non-zero to pyemu.os_utils.run() and an exception is raised, but then the test catches the exception like we want it to. Otherwise, an uncaught exception would be raised and the test would fail. clear as mud?!

briochh commented 2 months ago

So, I guess the subprocess option would remove the need to append that ".exe" string on windows? But it does require careful splitting of call strings to lists of args, right? Is that generalisable across use-cases -- with all possible exe and option combos? Not withstanding the potential can or worms, I am keen to see where such a shift might take us.

One thing to watch out for is maintaining how we deal with launching runs inside another directory and the logic behind where executables are on the PATH, from that other dir. - this has been a pain point for me when using the sim.run() methods in flopy.

jtwhite79 commented 2 months ago

was thinking about this - what if for now we have an optional kwarg that gets passed to pyemu.os_utils.run() like "use_sp" so that the os.system and sp.Popen styles can co-exist? This would give us time to take it out in the wild and try it on a range of cases. thoughts?

p-ortega commented 2 months ago

Sounds like a solid approach -- if ain't broke!

I’m up for giving it a shot and seeing how it works. I can try it out on a model I’m re-HM’ing soon and see how it goes