Add config datalad.ui.interactive and allow non-interactive special remotes #7344

bpoldrack · 2023-03-22T09:53:50Z

Makes is_interactive configurable via datalad.ui.interactive. Defaults to current behavior of detection via isatty(). The detection result is propagated to datalad subprocesses via env var (ping Passing configuration to subprocesses #7352).
Adds a non-dialog annex backend variant and makes the UISwitcher use it, based on is_interactive when asked to set the annex backend.

Closes #7345
Closes #7349

codecov · 2023-03-23T14:58:22Z

Codecov Report

Patch coverage: 87.50% and project coverage change: +1.97 🎉

Comparison is base (a8d7c63) 88.74% compared to head (93d5ac4) 90.71%.

❗ Current head 93d5ac4 differs from pull request most recent head 5417429. Consider uploading reports for the commit 5417429 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            maint    #7344      +/-   ##
==========================================
+ Coverage   88.74%   90.71%   +1.97%     
==========================================
  Files         327      327              
  Lines       44629    44649      +20     
  Branches     5913     5916       +3     
==========================================
+ Hits        39605    40505     +900     
+ Misses       5009     4129     -880     
  Partials       15       15

Impacted Files	Coverage Δ
datalad/__init__.py	`98.00% <ø> (+16.00%)`	⬆️
datalad/cli/tests/test_utils.py	`95.65% <ø> (ø)`
datalad/utils.py	`87.33% <62.50%> (-0.18%)`	⬇️
datalad/ui/tests/test_base.py	`98.30% <80.00%> (-1.70%)`	⬇️
datalad/ui/dialog.py	`92.70% <90.90%> (-0.31%)`	⬇️
datalad/cli/helpers.py	`80.50% <100.00%> (-0.17%)`	⬇️
datalad/cli/utils.py	`73.33% <100.00%> (ø)`
datalad/customremotes/__init__.py	`91.17% <100.00%> (ø)`
datalad/interface/common_cfg.py	`100.00% <100.00%> (ø)`
datalad/log.py	`88.75% <100.00%> (+0.04%)`	⬆️
... and 4 more

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mih

Just flew over the code, so this may be a bit too rough to be useful

datalad/__init__.py

datalad/interface/common_cfg.py

yarikoptic · 2023-03-25T16:47:48Z

with such extensive change touching functionality I also wonder if may be worth/safer for master?

bpoldrack · 2023-03-27T07:01:03Z

with such extensive change touching functionality I also wonder if may be worth/safer for master?

Main reason for maint to me is, that we (as in Jülich) need this released ASAP.

bpoldrack · 2023-03-27T11:54:17Z

I think, this is ready, @yarikoptic. There's the git-annex died of signal 11 on macos github action, but this seems unrelated.

yarikoptic

would break current interactions with users while installing/getting data from some datasets which do require authentication (e.g. i tried datalad install -g ///crcns/aa-1). I feel that there should be some "deeper" fix for interactivity detection or annex-no-dialog should be used only when explicitly requested with config setting (e.g. if explicitly asked to be non-interactive)
- note that we have datalad.tests.ui.backend config to control which UI backend to use during tests but we seems to have no datalad.ui.backend
for a bug fix, PR IMHO unnecessarily changes API by flipping from is_interactive function to a module attribute, and now confusingly imported from various levels (datalad and datalad.ui). Was there really a need for such a change if we are aiming for fixing some issue? (made review harder)
code duplication should be reduced IMHO

datalad/__init__.py

datalad/interface/common_cfg.py

datalad/tests/test_utils.py

yarikoptic · 2023-03-27T13:55:54Z

datalad/ui/__init__.py

-                backend = 'dialog' if is_interactive() else 'no-progress'
+                backend = 'dialog' if is_interactive else 'no-progress'
+        if not is_interactive and backend == 'annex':
+            backend = 'annex-no-dialog'


unfortunately this would break entirely ability of users to authenticate when possible... I have tested by removing crcns password and trying to datalad -l debug install -g ///crcns/aa-1 . I no longer is getting the prompt to enter the credentials for crcns to be able to download the data.

So I think the problem is in how we determine that session is interactive or not, and some (most? few? we don't know but so far we assumed that it is interactive) annex sessions are interactive although stdin is coming from git-annex so not a tty.

unfortunately this would break entirely ability of users to authenticate when possible

It doesn't break the ability enitrely, it changes it - you'd need to set the new config (as env var ATM). Because we are unable to correctly detect. And as far as I can tell, it's simply not possible. (see #7345)
Keeping the behavior of assuming interactivity by default, is not an option IMO, though. Because in non-interactive jobs, that just means stalling with no error reporting whatsoever. Users would have no clue at all.
Whereas in your case, there should at least be a hint that authentication wasn't possible. I'd rather err on the somewhat informative side than on the one that leaves users with nothing to act upon.

We could maintain the behavior to some extend, when we address #7352. Passing down the (accidently correct) detection from the super process, would lead to the same behavior in your case. I'd still argue it's wrong, since detection outside of a special remote process is not more correct. It's just slightly rarer to be an issue. (see #7354)

It doesn't break the ability enitrely, it changes it - you'd need to set the new config (as env var ATM).

well, not entirely means that it still breaks it regardless how you name it.

Not breaking at all would be whenever the default behavior stays as is at least for maint. Eg. could be done via config variable staying "auto" by default and then switch to True or False based on the utils.is_interactive(), while keeping using is_interactive() function, and allowing all CIs or whatnot which knows that there is no agent behind keyboard to say so. That would IMHO be better since it would be explicit and useful regardless on how we decide to trick/handle interactivity. Then in master we could be deliberated further or even change behavior.

Whereas in your case, there should at least be a hint that authentication wasn't possible.

why wasn't possible if it was and was working just fine before this change?

Eg. could be done via config variable staying "auto" by default and then switch to True or False based on the utils.is_interactive(), while keeping using is_interactive() function

That is what is happening. The special remote process evaluates the detection in the state of this PR. But detection (auto) simply doesn't work. It only happens to do the right thing, when the process in question is the top-level process. But we are querying from within the special remote process.

why wasn't possible if it was and was working just fine before this change?

It didn't "work" before that change. It happened to do the right thing in interactive sessions and the wrong thing in non-interactive ones, b/c it always assumed to be in an interactive one.

datalad/ui/dialog.py

datalad/utils.py

bpoldrack · 2023-03-27T16:16:19Z

@yarikoptic

I feel that there should be some "deeper" fix for interactivity detection

I spend hours on this and couldn't find one. That's why this solution.

or annex-no-dialog should be used only when explicitly requested with config setting

As I pointed out in the other spot: That means we are defaulting to stalling of non-interactive jobs.

Also: Does that mean, you want "regular" datalad processes to default to (broken) detection, but special remote process to default to assuming interactivity? This would mean to completely ignore, that this issue is not something special about special remotes. Any datalad process can find itself in the exact same situation as a special remote process.

The mitigation I can see, is #7352 - passing config manager's state down to subprocesses. So, if the top-level datalad process is set to default (detection) the result of that detection would be set for the special remote process as well (instead of running it's own detection which will always come back False).

datalad/ui/dialog.py

datalad/ui/tests/test_dialog.py

yarikoptic · 2023-03-27T19:14:04Z

The mitigation I can see, is #7352 - passing config manager's state down to subprocesses. So, if the top-level datalad process is set to default (detection) the result of that detection would be set for the special remote process as well (instead of running it's own detection which will always come back False).

yes -- if outside process datalad.ui.interactive set to "auto" , and it determines that it is interactive (or not), setting its env var DATALAD_UI_INTERACTIVE so any nested process also realizes that sounds like a viable way to go forward which would satisfy both of our desires?

edit: would need to set DATALAD_UI_INTERACTIVE envvar in any case, so config based options are overriden too in the children processes uniformly. What could go wrong? ;)

As I pointed out in the other spot: That means we are defaulting to stalling of non-interactive jobs.

... until realizing that the first stalled job is needing credentials (which it prompts for) and making sure that they have them in the future run which anyways would seems to be needed to do.

Again -- the other end of spectrum this PR introduces is that there is not prompt to the user who (may be due to incorrect assumptions and defaults) was getting a prompt and now all of a sudden would just get a crash that it cannot get to that data, and no instructions even on how to mitigate, or to mitigate by explicitly saying "be interactive" in the interactive shell.

bpoldrack · 2023-03-28T11:21:20Z

@yarikoptic

yes -- if outside process datalad.ui.interactive set to "auto" , and it determines that it is interactive (or not), setting its env var DATALAD_UI_INTERACTIVE so any nested process also realizes that sounds like a viable way to go forward which would satisfy both of our desires?

Here is a test commit: 5417429

would need to set DATALAD_UI_INTERACTIVE envvar in any case, so config based options are overriden too in the children processes uniformly.

Not sure, whether I parse that right. You mean, you'd need the env var to overrule anything set in config files? Yes, that, or -c or datalad.cfg.set(..., scope='override') (With above commit would be passed down either way)

yarikoptic · 2023-03-28T18:33:10Z

Not sure, whether I parse that right. You mean, you'd need the env var to overrule anything set in config files? Yes, that, or -c or datalad.cfg.set(..., scope='override') (With above commit would be passed down either way)

in effect - yes, it would overrule in the children processes, but not in the parent process where it would actually obtain initial value via any available means (config, env var, overwrites) and then set explicit bool value it into DATALAD_UI_INTERACTIVE so all subprocesses (and this parent process later on may be) use it. So, I do not immediately see any problem with the fact that children processes will not use value from the config value, and do not see a reason to utilize DATALAD_CONFIG_OVERRIDES_JSON for this specific aspect.

Note: this config is asked from the general datalad.cfg, i.e. not per dataset config.

bpoldrack · 2023-03-30T11:30:36Z

@yarikoptic

Note: this config is asked from the general datalad.cfg, i.e. not per dataset config.

Yes, I added this aspect to the changelog. Dataset.repo evaluation from within the runner seems to much of a performance hit for this feature. I guess, the need to have something passed to a subprocess, while requiring it to be specific to a dataset, is rare - can't think of anything ATM. So, currently I think this is the best of all the imperfect solutions.

bpoldrack · 2023-04-11T07:06:37Z

Verdict from devcall: Rip out generic passing down of configs again. Should only apply to datalad.ui.interactive for now.

special remotes This patch introduces the config `datalad.ui.interactive` in order to let users decide whether or not to run in interactive mode. The config defaults to the former detection, except that this detection is additionally safeguarded - any exception during that detection will lead to non-interactive mode. In addition, the result of the detection will be stored in `DATALAD_UI_INTERACTIVE`, thus passing down the result to possible subprocesses rather than having them running their own detection. Ultimately, this is about `ui`'s ability to talk to the terminal via `getpass` and the detection does not work from within subprocesses. Closes datalad#7349 Furthermore, this adds a non-interactive UI backend for annex special remotes, which previously always assumed to be in interactive mode, and adds the respective capacity for the UI_Switcher. Closes datalad#7345

bpoldrack · 2023-04-14T11:46:22Z

FTR: Remaining failures in crippledFS and macos github actions look as if #7367 wasn't merged.

Could it be that restarting github actions is running on an outdated, cached merge commit while Travis and AppVeyor build anew?

mih · 2023-04-26T07:37:35Z

Before this is merged (in particular the runner adjustment), I recommend looking at datalad/datalad-next#325 for an alternative -- which seems simpler, and also more effective.

This achieve the main goal of making any `datalad -c ...` specification affect not just the datalad-specific config in the main Python process, but can now handle *any* Git config, and also impact the behavior of any subprocesses. Furthermore this handling is extended to cover also `DATALAD_...` ENV variables, including `DATALAD_CONFIG_OVERRIDES_JSON`. Within the session `ConfigManager` instance the behavior is now more uniform. `ConfigManager.overrides` are now exclusively instance-specific overrides -- matching their description and implementation. No configuration override coming from CLI or process ENV is reflected in `ConfigManager.overrides` anymore. Closes datalad#325 -- although the scope is a bit broader. This changeset defers the need to address datalad#397, but does not resolve it. Ideally there would not be a need for any CLI specific behavior and implementation -- everything should be done by the `ConfigManager`. However, given the numerous conceptual and design limitations, it felt necessary to address the override impact limitation separately. Ping - datalad/datalad#4119 - datalad/datalad#3456 - datalad/datalad#7344

yarikoptic

some comments I found pending

yarikoptic · 2023-04-04T13:31:00Z

datalad/interface/common_cfg.py

+                    "can be wrong, though, possibly making datalad wait for "
+                    "user input, even though it is impossible to receive such "
+                    "input."}),
+        'default': is_interactive_failsafe(),


I thought I have expressed my opinion that here we better have "auto" instead of immediate definition, but I forgot why that would be better -- TODO

yarikoptic · 2023-04-04T13:38:24Z

changelog.d/pr-7344.md

+- Introduce new config `datalad.ui.interactive` to allow users to overrule detection of interactive sessions.
+  Faulty detection could lead to stalling, especially when subprocesses like git-annex special remotes where involved.
+  Fixes [#7345](https://github.com/datalad/datalad/issues/7345)
+  Fixes [#7349](https://github.com/datalad/datalad/issues/7349) via


Suggested change

Fixes [#7349](https://github.com/datalad/datalad/issues/7349) via

and [#7349](https://github.com/datalad/datalad/issues/7349) via

yarikoptic

more comments

yarikoptic · 2023-07-11T13:33:22Z

datalad/utils.py

+    try:
+        interactive = all(_is_stream_tty(s)
+                          for s in (sys.stdin, sys.stdout, sys.stderr))
+    except Exception as e:


I would worry about overall capturing of all exceptions here -- e.g. if detection breaks with new python or something like that -- we would miss it. Needs investigation of git history if more information is provided or just go back to no exception handling for now IMHO.

yarikoptic · 2023-07-11T13:34:44Z

datalad/utils.py

+        # Raise log level to DEBUG in this case, though.
+        CapturedException(e, level=logging.DEBUG)
+        interactive = False
+    os.environ['DATALAD_UI_INTERACTIVE'] = str(interactive)


I do not think it should overload if it is already defined.

yarikoptic · 2023-07-11T13:35:17Z

datalad/utils.py

@@ -369,14 +369,36 @@ def _is_stream_tty(stream: Optional[IO]) -> bool:


 def is_interactive() -> bool:
-    """Return True if all in/outs are open and tty.
+    from datalad import cfg
+    return cfg.obtain("datalad.ui.interactive")


what if config is within local git repo config?

shouldn't we overload that DATALAD_UI_INTERACTIVE here? and if we are to decide for "auto" to be default -- then here call is_interactive_failsafe explicitly?

synchon · 2023-11-21T14:38:41Z

Hi! I'm waiting for this fix to land in the main branch, is there an estimate when it can happen? It's a bit of a hassle to checkout a branch and then make it work. Thanks in advance! :)

bpoldrack force-pushed the fix-interactive-sr branch 6 times, most recently from 7b7db45 to e4ccb8c Compare March 23, 2023 14:21

mih reviewed Mar 23, 2023

View reviewed changes

datalad/__init__.py Show resolved Hide resolved

datalad/interface/common_cfg.py Outdated Show resolved Hide resolved

datalad/interface/common_cfg.py Show resolved Hide resolved

bpoldrack mentioned this pull request Mar 23, 2023

test_datalad_credential_helper - AttributeError: 'NoneType' object has no attribute 'credential' #7347

Closed

bpoldrack force-pushed the fix-interactive-sr branch 2 times, most recently from 87e25d6 to 6cfd40b Compare March 24, 2023 11:13

bpoldrack mentioned this pull request Mar 24, 2023

Passing configuration to subprocesses #7352

Open

bpoldrack changed the title ~~debug UnderAnnexUI~~ Add config datalad.ui.interactive and allow non-interactive special remotes Mar 24, 2023

bpoldrack force-pushed the fix-interactive-sr branch from 6cfd40b to 07a7b68 Compare March 24, 2023 15:21

bpoldrack mentioned this pull request Mar 24, 2023

Re-consider default interactivity detection #7354

Open

bpoldrack added the semver-patch Increment the patch version when merged label Mar 24, 2023

bpoldrack marked this pull request as ready for review March 24, 2023 15:52

bpoldrack force-pushed the fix-interactive-sr branch from 59dcd5f to 4febb59 Compare March 27, 2023 06:59

yarikoptic requested changes Mar 27, 2023

View reviewed changes

yarikoptic reviewed Mar 27, 2023

View reviewed changes

datalad/ui/dialog.py Outdated Show resolved Hide resolved

yarikoptic reviewed Mar 27, 2023

View reviewed changes

datalad/ui/tests/test_dialog.py Outdated Show resolved Hide resolved

bpoldrack force-pushed the fix-interactive-sr branch from 2a02fb1 to 93d5ac4 Compare March 27, 2023 19:27

bpoldrack force-pushed the fix-interactive-sr branch from 5417429 to b42469b Compare March 30, 2023 11:05

bpoldrack force-pushed the fix-interactive-sr branch 2 times, most recently from 72edace to fc0abd2 Compare March 30, 2023 11:30

bpoldrack marked this pull request as draft April 11, 2023 08:54

bpoldrack force-pushed the fix-interactive-sr branch from e685ece to 4311ed8 Compare April 11, 2023 10:04

bpoldrack marked this pull request as ready for review April 11, 2023 11:55

bpoldrack mentioned this pull request Apr 12, 2023

DataLad hangs waiting for input in non-interactive job psychoinformatics-de/knowledge-base#14

Open

4 tasks

mih mentioned this pull request Apr 26, 2023

Adjust ConfigManager to post overrides into the ENV datalad/datalad-next#325

Closed

mih mentioned this pull request May 2, 2023

Ought-to behavior of special remote implementations to enable progress logging #7382

Closed

mih mentioned this pull request May 31, 2023

Ensure that git is not interactive mode whenever DataLad is ran in non-interactive mode #7395

Open

mih mentioned this pull request Jun 2, 2023

Patch CLI to post any config overrides in Git-native fashion in ENV datalad/datalad-next#399

Merged

yarikoptic reviewed Jul 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config datalad.ui.interactive and allow non-interactive special remotes #7344

Add config datalad.ui.interactive and allow non-interactive special remotes #7344

bpoldrack commented Mar 22, 2023 •

edited

codecov bot commented Mar 23, 2023 •

edited

mih left a comment

yarikoptic commented Mar 25, 2023

bpoldrack commented Mar 27, 2023 •

edited

bpoldrack commented Mar 27, 2023

yarikoptic left a comment •

edited

yarikoptic Mar 27, 2023

bpoldrack Mar 27, 2023 •

edited

yarikoptic Mar 27, 2023

bpoldrack Mar 27, 2023 •

edited

bpoldrack commented Mar 27, 2023 •

edited

yarikoptic commented Mar 27, 2023 •

edited

bpoldrack commented Mar 28, 2023

yarikoptic commented Mar 28, 2023

bpoldrack commented Mar 30, 2023

bpoldrack commented Apr 11, 2023

bpoldrack commented Apr 14, 2023

mih commented Apr 26, 2023 •

edited

yarikoptic left a comment

yarikoptic Apr 4, 2023

yarikoptic Apr 4, 2023

yarikoptic left a comment

yarikoptic Jul 11, 2023

yarikoptic Jul 11, 2023

yarikoptic Jul 11, 2023

synchon commented Nov 21, 2023

	Fixes [#7349](https://github.com/datalad/datalad/issues/7349) via
	and [#7349](https://github.com/datalad/datalad/issues/7349) via

Add config datalad.ui.interactive and allow non-interactive special remotes #7344

Are you sure you want to change the base?

Add config datalad.ui.interactive and allow non-interactive special remotes #7344

Conversation

bpoldrack commented Mar 22, 2023 • edited

codecov bot commented Mar 23, 2023 • edited

Codecov Report

mih left a comment

Choose a reason for hiding this comment

yarikoptic commented Mar 25, 2023

bpoldrack commented Mar 27, 2023 • edited

bpoldrack commented Mar 27, 2023

yarikoptic left a comment • edited

Choose a reason for hiding this comment

yarikoptic Mar 27, 2023

Choose a reason for hiding this comment

bpoldrack Mar 27, 2023 • edited

Choose a reason for hiding this comment

yarikoptic Mar 27, 2023

Choose a reason for hiding this comment

bpoldrack Mar 27, 2023 • edited

Choose a reason for hiding this comment

bpoldrack commented Mar 27, 2023 • edited

yarikoptic commented Mar 27, 2023 • edited

bpoldrack commented Mar 28, 2023

yarikoptic commented Mar 28, 2023

bpoldrack commented Mar 30, 2023

bpoldrack commented Apr 11, 2023

bpoldrack commented Apr 14, 2023

mih commented Apr 26, 2023 • edited

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Apr 4, 2023

Choose a reason for hiding this comment

yarikoptic Apr 4, 2023

Choose a reason for hiding this comment

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Jul 11, 2023

Choose a reason for hiding this comment

yarikoptic Jul 11, 2023

Choose a reason for hiding this comment

yarikoptic Jul 11, 2023

Choose a reason for hiding this comment

synchon commented Nov 21, 2023

bpoldrack commented Mar 22, 2023 •

edited

codecov bot commented Mar 23, 2023 •

edited

bpoldrack commented Mar 27, 2023 •

edited

yarikoptic left a comment •

edited

bpoldrack Mar 27, 2023 •

edited

bpoldrack Mar 27, 2023 •

edited

bpoldrack commented Mar 27, 2023 •

edited

yarikoptic commented Mar 27, 2023 •

edited

mih commented Apr 26, 2023 •

edited