Runner -- shot of mercy #4996
Runner -- shot of mercy #4996
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4996 +/- ##
==========================================
- Coverage 90.15% 89.75% -0.40%
==========================================
Files 292 292
Lines 41002 40993 -9
==========================================
- Hits 36964 36795 -169
- Misses 4038 4198 +160
Continue to review full report at Codecov.
|
|
Ok, so this works in principle. Playing with the outcome, I think we could delete 2k lines of, now unused, code. However, the Runners are still not dead. The culprit after this PR is now |
There was a problem hiding this comment.
Thanks for doing this. It looks like quite the slog. Looking through (at f7af7ec), I didn't spot any problematic conversions, and -core's tests look happy.
I scratched my head at the -container failure (and associated PR) for a bit. If I understand it correctly, the reason that swallow_outputs works for Runner and not WitlessRunner is that Runner explicitly sets the stdout argument of Popen to sys.stdout, which allows the swallow_outputs override to be effective.
|
Thanks @kyleam for the analysis - I think that is indeed what is happening, and probably is a "feature". So datalad/datalad-container#131 should be finalized, merged, and new datalad-container released (we could merge this before but then we would end up closing our eyes on the -container failures in master possibly missing new regressions) |
| log_stdout=True, log_stderr=True, | ||
| expect_fail=True, expect_stderr=True) | ||
| toppath = toppath.rstrip('\n\r') | ||
| out = GitWitlessRunner(cwd=path).run( |
There was a problem hiding this comment.
a note: coverage marked this as not-covered for me -- doesn't make sense since it would be the first line in try: and prior lines are covered.
Just to lend more concrete support to my explanation: demoimport subprocess as sp
import sys
from datalad.cmd import Runner
from datalad.cmd import WitlessRunner
from datalad.utils import swallow_outputs
def old_runner():
Runner().run(["pwd"], log_online=True,
log_stdout=False,
log_stderr=False)
def new_runner():
WitlessRunner().run(["pwd"])
def popen():
p = sp.Popen(["pwd"])
p.communicate()
def popen_with_sys_stdout():
p = sp.Popen(["pwd"], stdout=sys.stdout)
p.communicate()
for fn in [old_runner, new_runner, popen, popen_with_sys_stdout]:
print(f"===== {fn.__name__}")
with swallow_outputs() as out:
fn()
x = out.out
print("----- swallowed")
print(x) |
|
Given that this PR isn't achieving its goal (total removal of Runner) anyways, I am OK with stripping the change to |
If `cmd` is a string, it is executed via the platform shell.
This now additionally tests that the NoCapture protocol enabled interactive job execution. This is (or will be) critical for enabling efforts like dataladgh-4121 (stalled). Ping dataladgh-3236
Reasoning inside. Context: datalad#4996
|
Thx @kyleam ! |
datalad/datalad#5002 switches `datalad run` over to using WitlessRunner rather than the old Runner. swallow_outputs() works for Runner because it explicitly sets the stdout argument of Popen() to sys.stdout, which allows the swallow_outputs() override to be effective. This is not the case for WitlessRunner. A few commits back already adjusted datalad_container/tests/test_run.py for the above change in behavior. Use the same approach in the remaining spots that swallow and check the output of a containers_run() call. datalad/datalad#4996 (review)
All Runner code is now hanging on a thin thread. The remaining connections are:
GitRepo._run_git_custom_commandis still on life support but already brain deadrunstill usesRunner-> RF: Stop using old-Runner inrun#5002create_siblinghas a fancyRunneradaptor class that adds a few functions that partially duplicate functionality inSSHConnectionand want to become command abstractions (although that effort NF: Command abstractions #4068 died a lonely death, @bpoldrack )Significant changes: