You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/28 19:02:48 UTC
[GitHub] [airflow] potiuk opened a new pull request, #23335: Unify approach for user questions asked in Breeze
potiuk opened a new pull request, #23335:
URL: https://github.com/apache/airflow/pull/23335
This change documents and unifies the approach we've taken for
the user inut handling when it comes to confirmation questions.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112682599
BTW. One more thing.
With the timeout - you have not accounted for two things.
1) the message is going to be repeated multiple times. when people will run it. So what I am counting on is the time of displaying the message to be multiplied by the number of times people see it.
2) You have not taken into account scrolling the terminal back. Your logic breaks when you proposed "just print warning". If we follow your min 30 seconds and take it literaly, this means that any warning printed is never read. If you take it to extreme, you should never print any message to the user that might potentially lead to an action from the user side. Which is absurd when it comes to CLI. When peopel will experience errors later, they will likely scroll up to see what was there before - or - in the worst case copy&paste the output if we ask them to. So they will be able to see that message, read and ponder if the fact they did not answer "y" might be the reason for failure. And they might try again. This sis what I really want to induxe here. This is subtlle and not 0/1 but well, this is playing to human psychology and it's rarely 0/1 there are shades og gray.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1116554675
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112571079
I totally skipped over the ADR, apologies. Reading now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112629969
> So to sum up my point: if we're going to go through the trouble of asking the user for input at all, we should actually care about their informed consent/response.
We are not asking for consent in 3) - we give the user little time to revert the decision that is already made for them as "good default" option. It has nothing do do with consent. For consent, point 1) is already addressed with "no timeout".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112622288
Based on your comments @blag I added a few things:
* increased timeout to 5 seconds to give a bit more time to answer
* added instructions to power-users "You can add `--answer n` to skip the delay:" . This will give the power users quick info on how they can avoid the delay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112611198
> Case 3: Prompting for a decision when auxiliary actions should maybe be taken. 👎 No, this is bad UX.
I Disagree. I want to make users to upgrade when they can. To be annoying when they did not upgrade but continue nagging them . This is the goal. If you have another proposal how to achieve that - I am all ears. Warnings printed without annoying delays do not work. People ignore them.
> Power users should be able to set behavior using environment variables or flags (see case 2) - this is exactly how docker build --no-cache works, and they shouldn't be delayed by a timeout with a default answer - that just adds needless delay to their day.
This is precisely what I want. I am targetting this for "non-power users". Power users already have an option to skip the delay - they can add ``--answer no``. So there is an escape hatch for power users to run stuff without delay.
I want "no upgrade" to be annoying for casual users when they need to upgrade. But I do not want them to break them the workflow. This is for example what NPM does and I really lilke it when I saw it (this is where the idea of timout came from):
```
================================================================================
================================================================================
DEPRECATION WARNING
Node.js 10.x is no longer actively supported!
You will not receive security or critical stability updates for this version.
You should migrate to a supported version of Node.js as soon as possible.
Use the installation script that corresponds to the version of Node.js you
wish to install. e.g.
* https://deb.nodesource.com/setup_12.x — Node.js 12 LTS "Erbium"
* https://deb.nodesource.com/setup_14.x — Node.js 14 LTS "Fermium" (recommended)
* https://deb.nodesource.com/setup_16.x — Node.js 16 "Gallium"
Please see https://github.com/nodejs/Release for details about which
version may be appropriate for you.
The NodeSource Node.js distributions repository contains
information both about supported versions of Node.js and supported Linux
distributions. To learn more about usage, see the repository:
https://github.com/nodesource/distributions
================================================================================
================================================================================
Continuing in 20 seconds ...
```
If we are confident enough to take action after a timeout, then we should be confident enough to just emit an info or warning message (and use Rich's colors for that! 😄) and carry on with that action without any delay.
This is not 0/1 . People will ignore it mostly. As a maintainer, I want "no-upgrade" to be annoying and make the people eventually click. Po
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112649527
> Then we should at least give the user time to read it before carrying on. Doing so will also hopefully get them to understand what the command is doing.
I pushed a fixup where "timeout=None" is the default, and I also introduced a STANDARD_TIMEOUT=10 seconds.
I also removed timeout in one case when it could do without it.
Plus I added description to inform the users about consequences of their action and what they might expect if they don't (all concise and brief). I think this is the best response to:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1113957199
Anyone :) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1113136194
OK. I added the last comment - reviewed the messaging and made it a bit cleaner.
Clearly we can't get to agree, I think there are were many valuable comments that I took into account in it and I am super happy to merge this version now..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1113145795
Would love to get other's to comment.
Some of the screenshots:
Auxiliary "upgrade needed" when you run breeze command:
Regular "no-timeout" when action is requested by the user (but operation is potentially distuptive):
Accidenatl "breeze" upgrade prompt with timeout:
I believe it's as good as it can be :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #23335:
URL: https://github.com/apache/airflow/pull/23335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23335:
URL: https://github.com/apache/airflow/pull/23335#discussion_r861227623
##########
dev/breeze/src/airflow_breeze/commands/custom_param_types.py:
##########
@@ -53,6 +54,17 @@ def get_metavar(self, param) -> str:
return f"[{choices_str}]"
+class AnswerChoice(BetterChoice):
+ """
+ Stores forced answer if it has been selected
+ """
+
+ name = "AnswerChoice"
+
+ def convert(self, value, param, ctx):
+ set_forced_answer(value)
Review Comment:
This allowed us to remove manual "set_forced_answer" wherever `--answer` was used :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112558227
cc: @blag -> this is the change that timeout behaviour is documented (I wrote the ADR) and fixed in a few places to follow it better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112590254
> What is the rationale behind making timeout values be single digit seconds?
In case 3) - we want to gently nudge the user to run an action when we guess it is advised. We should not
break the regular workflows, but we want the user to "eventually" run the upgrade to use the same
environment as others (and avoiding the problem where thing "work for me but not the others".
> For somebody who knows a priori what the questions are going to be and the implications of their answers are, they can probably predict or at least quickly recognize what the text is saying and respond that quickly. But for anybody who needs extra time to read things (hint: people with reading difficulties and/or disabilities), for newbies who need to read and make a decision, or for people who just like to read what their computer is saying to them (which is a practice that we should support!), single digit timeouts are far too short, in an extreme way.
This is fine. There is nothing wrong if they don't answer. This is expected and good outcome if they don't react. That's also desired in most cases. But it's also desired to be "slightlly annoying" and a) when they see it multiple times they will answer it 'y' eventually when they have time. This is precisely the behaviour I want to promote.
> I'm not a fan of timeouts, period. If something is important enough to display to the user and request the user's input on, it's important enough to require the user's input. Otherwise we should just emit a warning and carry on.
Nope. This is not binary. There is also "eventual consistency". Things are "the same", "differnt" and "eventually consistent" (i.e. wait or repeat enought time and you transition from "no" to "yes").
> But on top of that, forcing default answers if the user doesn't answer within a timeout is also something that should not be standard practice, and should not be commonly done. Asking the user for input, and then ignoring their lack of input is tantamount to just guessing at their intent. And the Zen of Python has something to say about that: ["In the face of ambiguity, refuse the temptation to guess."](https://peps.python.org/pep-0020/)
I think in this case it makes perfect sense. We did not have timeouts and it was extremely annoying. I don't see ambiguity. There is no ambiguity here. The message is simple "You can upgrade but if you don't decide, we will skip it" . The updated message is very clear about it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112602307
Case 1. Agree. Double check with the user before doing anything dangerous or destructive that we can't easily undo. _Acquire the user's informed consent_ before doing anything irreversible. There should be no timeout in this case. :+1:
Case 2. Not asking questions when a user has explicitly asked for a long-running command or an otherwise disruptive operation: :+1:. At that point the user is likely aware of the implications and has _explicitly consented to them_. In the case of an unexpected condition, a timeout is (IMO) the wrong way to handle that. If the timeout default answer is "quit", then we should not bother asking the user anything, just emit a message, (this next part is really important:) let the user know all of the ways to force the command to run, and then quit, all without waiting for a timeout. We should support using environment variables to set the timeout default answer (similar to Apt's `DEBIAN_FRONTEND` or how Homebrew handles this), as well as utilizing some sort of `--yes` or `--force` flag (similar to Docker's `docker build --no-cache`).
Case 3: Prompting for a decision when auxiliary actions should maybe be taken. :-1: No, this is bad UX. Power users should be able to set behavior using environment variables or flags (see case 2) - this is exactly how `docker build --no-cache` works, and they shouldn't be delayed by a timeout with a default answer - that just adds needless delay to their day. If we are confident enough to take action after a timeout, then we should be confident enough to just emit an info or warning message (and use Rich's colors for that! 😄) and carry on with that action without any delay. Something like:
> Airflow images haven't been rebuilt in awhile. If you run into issues, update your branch against `main` and rebuild images with `...`.
Given all of these use cases, I don't really see much of any benefit to using timeouts anywhere. And if something is important enough to ask the user for input, we should at least give **all** users, even those with reading/literacy difficulties, users with disabilities, and new users, enough time to read, think about, and respond to our prompts. IMO, timeouts are mostly an anti-pattern <sup>1</sup>.
<sup>1</sup> There are exceptions to this. (1) On macOS' and Ubuntu's GUIs if you go to logout or shutdown your computer, the confirmation screen will show you a 60 second timeout. This is a security improvement, because it helps when people click everything but the confirmation button. And it's not disruptive, because it both takes multiple, deliberate clicks to get to that screen in the first place, and if a user reaches that screen by accidentally mis-clicking, it's visually very obvious that it's not what they were actually trying to do. (2) When asking your phone to navigate somewhere via voice command, both Google Maps and Apple Maps will pull up an interstitial page to confirm your destination, and that page has a timeout and will automatically begin navigating there if you do not cancel it. This is useful because voice commands are usually done when people are driving, and throwing a timeout in there allows the user to drive their car and simply wait for (IIRC, 10 seconds)
instead of directing their attention to hunt for the "GO" button on their phone. Neither of these cases apply to Airflow, but they are exceptions to the rule, hopefully they give a better idea of how timeouts can be used in non-anti-pattern ways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112568162
What is the rationale behind making timeout values be single digit seconds?
For somebody who _knows_ a priori what the questions are going to be and the implications of their answers are, they can probably predict or at least quickly recognize what the text is saying and respond that quickly. But for anybody who needs extra time to read things (hint: people with reading difficulties and/or disabilities), for newbies who need to read and make a decision, or for people who just like to read what their computer is saying to them (which is a practice that we should support!), single digit timeouts are far too short, in an extreme way.
I'm not a fan of timeouts, period. If somebody is important enough to display to the user and request the user's input on, it's important enough to require the user's input. Otherwise we should just emit a warning and carry on.
But on top of that, forcing default answers if the user doesn't answer within a timeout is also something that should not be standard practice, and should not be commonly done. Asking the user for input, and then ignoring their lack of input is tantamount to just guessing at their intent. And the Zen of Python has something to say about that: ["In the face of ambiguity, refuse the temptation to guess."](https://peps.python.org/pep-0020/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112626838
Having a delay for a command - without asking the user anything - is okay IMO. Annoying the user into keeping up-to-date is verging into Dark Pattern territory but isn't there (and also, keeping up-to-date isn't a bad thing, and definitely helps the user avoid further issues).
Good to hear that there are escape hatches for power users. :+1:
If we're targeting non-power users, and if we're prompting users for input, then we should at least give the user time to read it before carrying on. Doing so will also hopefully get them to understand what the command is doing, which will aid in onboarding users. Bulldozing over their consent anyway just trains users to blindly run commands without understanding what the commands are doing. That's not helpful to anybody.
So to sum up my point: if we're going to go through the trouble of asking the user for input _at all_, we should actually care about their informed consent/response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112631794
> If we're targeting non-power users, and if we're prompting users for input, then we should at least give the user time to read it before carrying on.
What timeout do you think is enough ? I am happy to increas that if you find it annoying - maybe I know a bit "too well" what's going on here - but I am happy to change it to say 10 seconds? Do you think it is enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112730294
> 1. the message is going to be repeated multiple times. when people will run it. So what I am counting on is the time of displaying the message to be multiplied by the number of times people see it.
This just seems like worse UX, but okay. 🤷
> If we follow your min 30 seconds and take it literaly, this means that any warning printed is never read and understood (because people had no time to read it).
I apologize if I haven't been clear enough. I proposed a few different scenarios.
The easiest scenario is to simply emit a colorful warning and let the user discover it if/when things fail. The steps for that is:
1. Print a colorful warning ("Breeze is out-of-date. If this fails, try updating breeze with (`breeze self-update`) and run this again")
2. Carry on.
Another scenario is when we're just trying to delay/annoy the user into updating breeze, or pulling in changes from `main`, or whatever - cases where we don't actually need or care about user input, but we do want to apply some pressure. The steps in that case are:
1. Print a colorful warning (eg: "If this fails, try updating breeze or Airflow and try again. You can disable this wait by setting `DONT_WAIT_JUST_GO=...` or specifying the `--dont-wait-just-go` option")
3. Wait for a delay (this does not have to be 30 seconds, it can be lower)
4. Continue with what the command doing in the first place (eg: Run breeze without updating)
The user has to presumably stare at the warning for the duration of the delay, presumably reading and pondering over the correct course of action to take. And if they do not, and the command continues and eventually fails, the user has hopefully taken note of the warning and tries the command again with changes suggested from the warning (eg: pulling in changes from `main`, setting an environment variable and rerunning the command, or running the command with an additional flag). And even in the case that the user completely ignore the warning, switches context to something else during the delay, and eventually switches back and sees that the command has failed, they can scroll right back up to where the warning is printed in nice colors (bring the users attention to it), and that will hopefully give them enough of a cue to implement the suggested changes and try the command again. Since we don't actually care about the user's input here, we don't bother asking them for it.
The next scenario is when we do actually care enough to ask for user input. In this case, I think we should avoid timeouts if at all possible. In that case, the step would be:
1. Ask for user input and hang until they answer (eg: "Recursively delete '/'? y/n")
Finally, as a compromise, or in a such a case that we must timeout while waiting for input (although I really can't think of a good use case for that myself, I'm willing to consider that the possibility may exist), we can add a timeout of 30 seconds to give the user time to read the text, ponder their decision, and either cancel the command (to presumably immediately rerun it with changes) or cajole the command into continuing. Those steps would be:
1. Ask for user input, including wording to indicate how to run the command in such a way to bypass the need to ask for user input
2. Wait for the user to answer, and proceeding with a default answer upon timeout (minimum: 30 seconds)
3. Do whatever is dictated by the answer, or timeout/default answer, as appropriate
Waiting for a minimum timeout of 30 seconds here allows the user to read what's going on and perform various checks to validate that they're in a good position to continue. Checks like:
* Am I on wifi or did my laptop tether to my phone with it's highly throttled/expensive connection? My wifi coverage has been spotty recently and when it does my laptop "helpfully" likes to automatically switch to using my phone's connection.
* How big is the diff between my branch and `main`? If it's too big, I may not want to pull in changes yet. If it's tiny, then yes, pull in changes and run again.
* Is the Docker daemon running? Oh right, I disabled it from automatically starting on my old MacBook because it spins up the fans, but that means I have to manually start it and I haven't done that yet. Gotta do that, and Docker takes a bit to start on my old laptop.
* Wait, I just pulled in changes it shouldn't be asking about this! Oh wait, whoops, no, I wanted to run this command in my other terminal, in my other Airflow repository.
Every item of that mental checklist can take time, and it's very presumptive that everybody running Breeze can tick off everything in that checklist within a short timeout (eg: 10 seconds). Even 30 seconds is still majorly presumptive, which is why I said that it should be minimum.
> Since you are proposing a change, I suggest you come up with and experiment and gathering some data to prove your opinion is justified as I think we have just different opinion on the subject.
If that's what it's going to take to make this change, then I'm just going to give up and disengage from this discussion. That would be a very large amount of effort to change this. I highly doubt that the original author of this code went through such a study when selecting the timeout values, so I think it's a bit unfair to ask somebody to go through so much effort to change something that is (if I have adequately explained it here) pretty obvious and non-subtle IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1113101596
>
> This just seems like worse UX, but okay. shrug
I'd say it's message reinforcing. It's a very well known technique in UX to remind people about something several times. It has various degrees of complexity (when and how to remind), but reinforcing message by repeating it is pretty good to make people into doing what you want (and yes the whole UX here is an attempt to make people what we want - eventually upgrade and make them aware that error might have been caused by their inaction to defer upgrade). It's realy putting responsibilty in the user's hand.
> 1. Print a colorful warning ("Breeze is out-of-date. If this fails, try updating breeze with (`breeze self-update`) and run this again")
> 2. Carry on.
Yeah. This is what `pip` tries to do. And i''ts rather inefficient. I've been following `pip` story for quite a while. And they keep on complainign that user do not follow the warnings they issue.
> Another scenario is when we're just trying to delay/annoy the user into updating breeze, or pulling in changes from `main`, or whatever - cases where we don't actually need or care about user input, but we do want to apply some pressure. The steps in that case are:
>
> 1. Print a colorful warning (eg: "If this fails, try updating breeze or Airflow and try again. You can disable this wait by setting `DONT_WAIT_JUST_GO=...` or specifying the `--dont-wait-just-go` option")
> 2. Wait for a delay (this does not have to be 30 seconds, it can be lower)
> 3. Continue with what the command doing in the first place (eg: Run breeze without updating)
The problem with this scenario is that by the time the action of the user completes, they will forget about upgrading. They have already done the task. They don't care about fixing the warning they saw - even if it caused a delay. By giving an opportunity to the user to react at the place and time they are warned, is the exact place where their attention is focused. And they can act immediately - with an immediate reward. This is the same mechanism that controls impulse buying - and it is a well known technique if you want to make people do something. Act on impulse. All the marketing studies say that if you wait even short time before the offer and opportunity for an action, the conversion rate drops dramatically. This "react now" possiblity is there precisely for that reason - increase conversion rate in marketing terms.
> 1. Ask for user input and hang until they answer (eg: "Recursively delete '/'? y/n")
You do not know the full history. Tt has been exactly like that for ~ 1.5 year before I changed to timeout based on observations, getting feedback from people using it, asking them actively on slack and my own experiences with using breeze. So switching to timeout was a result of that and learnings i had and a year long experiment I run.
> Waiting for a minimum timeout of 30 seconds here allows the user to read what's going on and perform various checks to
validate that they're in a good position to continue.
Not sure where the 30 seconds are from. Can you state what it is based on, really ?
> * Am I on wifi or did my laptop tether to my phone with it's highly throttled/expensive connection? My wifi coverage has been spotty recently and when it does my laptop "helpfully" likes to automatically switch to using my phone's connection.
The decision might be taken for you in this case. And you will do all the mental effort. But next time when it repeats it's going to be faster.
> * How big is the diff between my branch and `main`? If it's too big, I may not want to pull in changes yet. If it's tiny, then yes, pull in changes and run again.
This check is already done automatically. When you are asked the question, we already checked and we are sure the chages are significant enough. This is one thing that we want to "teach" people by reinforcing the message. Again - the interface here is also designed to "teach" people and get them learn it over time. But there is no big harm if you don't make the decision.
> * Is the Docker daemon running? Oh right, I disabled it from automatically starting on my old MacBook because it spins up the fans, but that means I have to manually start it and I haven't done that yet. Gotta do that, and Docker takes a bit to start on my old laptop.
If the docker daemon not running you will not get here. It woudl fail much earlier at "docker check".
> * Wait, I just pulled in changes it shouldn't be asking about this! Oh wait, whoops, no, I wanted to run this command in my other terminal, in my other Airflow repository.
Oh yeah. That's why we print important information during "self-upgrade" about the "other repository" so that you can make decision faster. That's actualy a good point to add this information to the message with image building. Fixup is coming.
> Every item of that mental checklist can take time, and it's very presumptive that everybody running Breeze can tick off everything in that checklist within a short timeout (eg: 10 seconds). Even 30 seconds is still majorly presumptive, which is why I said that it should be minimum.
The idea is that they eventually will. Not the first time, not the second, but aafter 10th time they will. And that's fine to wait. I am not looking for "immediate" results. I think in much longer timespan and take into acount the fact that people get similar message multiple time and learn. This is what my goal is - to also let people learn over time and support it.
> If that's what it's going to take to make this change, then I'm just going to give up and disengage from this discussion. That would be a very large amount of effort to change this. I highly doubt that the original author of this code went through such a study when selecting the timeout values, so I think it's a bit unfair to ask somebody to go through so much effort to change something that is (if I have adequately explained it here) pretty obvious and non-subtle IMHO.
It took initially 1.5 years of running in the mode 3) you proposed and based on this 1.5 year experiment, it took the author (me) to make the decisions about the timeout. And I reverted my original decision (which was similar to your proposal) by learning. So yes - I run such an experiment for quite some time and thought a lot an analyzed people usage.
Let me ask you - how long is your experiment going to think that your proposal is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112674362
> And yes, I think you are intimately familiar with Breeze so you can probably recognize exactly what the prompt is just by the length, shape, and color of the lines Breeze is spitting. You probably don't even need to read the words on the screen, and/or can digest it line by line instead of word by word. Other folks, maybe not so much.
I will leave 10 seconds as compromise and see what people think. It's been 4 second for the last year or so.
Since you are proposing a change, I suggest you come up with and experiment and gathering some data to prove your opinon is justified as I think we have just different opinion on the subject.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blag commented on pull request #23335: Unify approach for user questions asked in Breeze
Posted by GitBox <gi...@apache.org>.
blag commented on PR #23335:
URL: https://github.com/apache/airflow/pull/23335#issuecomment-1112666622
> What timeout do you think is enough ? I am happy to increas that if you find it annoying - maybe I know a bit "too well" what's going on here - but I am happy to change it to say 10 seconds? Do you think it is enough?
For instances where we care about user input: 30 seconds, minimum. That sounds like a lot, but for somebody who has to read, ponder, and respond, it's not.
If all we want to do is add a delay, then I think we should just add a delay and _not_ prompt for user input. Folks who know what they're doing can Ctrl+C out of it and rerun with the appropriate flag, just like they likely do for `npm`, `apt`, `brew`, etc. And if we're just adding a delay, I mean, npm thinks 20 seconds is a good wait time for that, so I would probably start with that time and play around with it from there.
And yes, I think you are intimately familiar with Breeze so you can probably recognize exactly what the prompt is just by the length, shape, and color of the lines Breeze is spitting. You probably don't even need to read the words on the screen, and/or can digest it line by line instead of word by word. Other folks, maybe not so much. 😆
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org