Migration script: migrate with user input #3044

cphyc · 2021-01-27T21:26:24Z

PR Summary

At the moment, if a user has ~/.config/yt/ytrc but no ~/.config/yt/yt.toml, a hard-exit happens which prevent yt from being used. Though we display an explicit message advising the user on how to fix the issue, we may give the user a chance to interactively perform the migration immediately to lower the friction of this transition.

Xarthisius · 2021-01-28T00:00:53Z

If I updated yt on a cluster and run a batch job, I'd be very unhappy that my CPUh were spent on waiting on input(). I'd be infinitely less unhappy if it died right away.

matthewturk · 2021-01-28T00:02:15Z

Ok, this is an extremely good point. Let's brainstorm solutions.

…

On Wed, Jan 27, 2021, 6:01 PM Kacper Kowalik ***@***.***> wrote: If I updated yt on a cluster and run a batch job, I'd be very unhappy that my CPUh were spent on waiting on input(). I'd be infinitely less unhappy if it died right away. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3044 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO4PSQ3YQITKPAFJHMLS4CSMJANCNFSM4WV6WJWA> .

Xarthisius · 2021-01-28T00:03:29Z

Can we just do it like we do with everything else? Start with deprecation warning over the course of one release, then die if not fixed after next + 1 release?

neutrinoceros · 2021-01-28T08:05:32Z

The point was to protect users from ignoring the warning + wondering what went wrong later. In all honesty, I'm reconsidering if it's even worth it. Thanks @Xarthisius for bringing this up. It actually happened to me once and I confirm it didn't make me a fan of the code in question...

Can we automatically perform the migration BUT keep the original file without appending ".bak" to it ?
Then both confit files would be present and we'd raise a non blocking warning at load time until the old one is (re)moved.

cphyc · 2021-01-28T09:35:14Z

I have made a recap of all the solutions that emerged out of the discussion. Please feel free to edit the pros/cons so that we converge relatively quickly.

#	Solution	Pros	Cons
👍	Stop early with a `raise SystemExit` (current behaviour)	Ensures users perform the migration	We may receive many complaints about yt "not working"
🚀	Raise warning then error in yt 4.1.0	Transition is smooth for the users	Some users may ignore the warning and not perform the migration, yet still think `~/.config/yt/ytrc` is their config file
🎉	Raise warning, migrate automatically	Transition is even smoother for the users	We're modifying on-disk files without user's consent
❤️	Raise warning, migrate automatically after confirmation	Transition is smooth	Will result in annoying behaviour if launched on a non-interactive environment (ex. on a cluster) as we're using for user input

Please vote for your favourite solution by flagging this message with the corresponding emoji.

neutrinoceros · 2021-01-28T09:55:51Z

I voted 🎉 because the "cons" is actually aligned with yt's old behaviour: we used to create a ytrc file when none was found, BUT I actually don't like this "feature" very much (for the exact reason you're mentioning).
I think it is reasonable to migrate automatically provided two restrictions are followed

do not write to yt.toml if the file already exists
do not create a config file ex-nihilo, only do it if the old one is present.

Again, if this turns into a monstrosity of maintainability, my second preferred option is ":rocket:"

cphyc · 2021-01-28T10:05:31Z

Regarding

I voted tada because the "cons" is actually aligned with yt's old behaviour: we used to create a ytrc file when none was found, BUT I actually don't like this "feature" very much (for the exact reason you're mentioning).
I think it is reasonable to migrate automatically provided two restrictions are followed
1. do not write to yt.toml if the file already exists

2. do not _create_ a config file ex-nihilo, only do it if the old one is present.
Again, if this turns into a monstrosity of maintainability, my second preferred option is "rocket"

Regarding your points:

currently, the error is only triggered iff ytcfg exists but yt.toml does not. If both are present, there's only a warning.
this would imply a larger change (but why not) as currently yt creates an empty config file if none is found. Note that this behaviour is only here because I left it in Migrate config to toml #2981, but is not necessary per se. Another point: the current issue is only affecting users who aren't new to yt. They should all have a ytrc file since it is created automatically. For new users, there's no problem as they don't have a ytrc file.

neutrinoceros · 2021-01-28T10:10:28Z

currently, the error is only triggered if ytcfg exists but yt.toml does not. If both are present, there's only a warning.

excellent

the current issue is only affecting users who aren't new to yt. They should all have a ytrc file since it is created automatically. For new users, there's no problem as they don't have a ytrc file.

Fair enough, I didn't connect the dots.

matthewturk · 2021-01-28T10:47:50Z

Are we also putting folks into a weird spot if they have both a 3.x and 4.x installation present?

cphyc · 2021-01-28T10:59:24Z

Are we also putting folks into a weird spot if they have both a 3.x and 4.x installation present?

Currently, the only problematic case is when ~/.config/yt/ytrc exists, ~/.config/yt/yt.toml does not and users are on the main branch of yt.

If both config files exists, there is a warning and nothing else.

matthewturk · 2021-01-28T13:56:44Z

OK, that actually sounds like an OK solution to having both, to me...

…

On Thu, Jan 28, 2021 at 4:59 AM Corentin Cadiou ***@***.***> wrote: Are we also putting folks into a weird spot if they have both a 3.x and 4.x installation present? Currently, the only problematic case is when ~/.config/yt/ytrc exists, ~/.config/yt/yt.toml does not *and* users are on the main branch of yt. If both config files exists, there is a warning and nothing else. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3044 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXOZRITK7OLGVPWGFX6LS4E7RZANCNFSM4WV6WJWA> .

cphyc · 2021-01-28T14:13:58Z

@matthewturk @Xarthisius @neutrinoceros I have created two other PRs (#3047 and #3046) that implement the automatic migration and the simple warning solution respectively. Could you have a look and tell me which one should be kept?

neutrinoceros · 2021-01-28T14:32:40Z

#3047 is my preferred option, given some further changes.

munkm · 2021-02-25T20:04:03Z

It seems like 🚀 is winning the poll?

neutrinoceros · 2021-02-25T20:20:35Z

Yup. As the only advocate for a different option I gracefully resign. I think the poll was open long enough that everyone who wanted to vote already did. Moving forward ? :)

munkm · 2021-02-25T20:23:32Z

I think 🚀 was still your second choice option though!

Moving forward ?

I support it!

neutrinoceros · 2021-02-25T20:28:18Z

It was. That's why I'm gracefully resigning, instead of being this guy.

cphyc · 2021-02-26T09:15:33Z

Closing this now in favour of #3046.

neutrinoceros

Oops, forgot to push the "submit" button yesterday. Might as well publish this review even if the PR is dead

neutrinoceros · 2021-02-25T21:56:06Z

yt/utilities/configure.py

+    prompt = "Perform the migration now [yn]? "
+    user_input = input(prompt).lower()
+    while user_input not in ("y", "yes", "n", "no"):
+        print(f"Did not understand your input '{user_input}'. Please enter 'y' or 'n'.")
+        user_input = input(prompt).lower()
+
+    if user_input in ("y", "yes"):
+        migrate_config()
+    else:
+        raise SystemExit("Migration not performed: exiting.")


Obviously a semi-joke because we don't have rich as dep, but let's imagine for a second we do:

Suggested change

prompt = "Perform the migration now [yn]? "

user_input = input(prompt).lower()

while user_input not in ("y", "yes", "n", "no"):

print(f"Did not understand your input '{user_input}'. Please enter 'y' or 'n'.")

user_input = input(prompt).lower()

if user_input in ("y", "yes"):

migrate_config()

else:

raise SystemExit("Migration not performed: exiting.")

from rich.prompt import Confirm

migrate = Confirm.ask("Perform the migration now ?")

if not migrate:

raise SystemExit("Migration not performed: exiting.")

migrate_config()

(rich is awesome)

More seriously, I think this is fine but I'm still haunted by @Xarthisius' remark: this would be orders of magnitude better with a timeout. Problem is I have no idea how to implement it and 5min of stackoverflowing didn't help much.

neutrinoceros · 2021-02-25T22:03:40Z

yt/config.py

@@ -209,7 +209,9 @@ def get_local_config_file():
                since="4.0.0",


this message requires some tweaking, since running the shell command isn't required after all.

Query the user instead of failing hard

cb3c8f4

cphyc changed the title ~~improve-migration-script~~ Improve migration script Jan 27, 2021

Will the fix the circular import?

b2ae0c7

cphyc added the enhancement Making something better label Jan 27, 2021

neutrinoceros approved these changes Jan 27, 2021

View reviewed changes

neutrinoceros mentioned this pull request Jan 28, 2021

Unnecessary config file creation #3045

Closed

cphyc changed the title ~~Improve migration script~~ Migration script: migrate with user input Jan 28, 2021

This was referenced Jan 28, 2021

Migration script: migrate automatically #3047

Closed

Migration script: warn and do not migrate #3046

Merged

cphyc requested a review from neutrinoceros January 28, 2021 14:14

cphyc added the yt-4.0 feature targeted for the yt-4.0 release label Feb 19, 2021

cphyc closed this Feb 26, 2021

neutrinoceros reviewed Feb 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration script: migrate with user input #3044

Migration script: migrate with user input #3044

cphyc commented Jan 27, 2021 •

edited

Loading

Xarthisius commented Jan 28, 2021

matthewturk commented Jan 28, 2021 via email

Xarthisius commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

cphyc commented Jan 28, 2021 •

edited

Loading

neutrinoceros commented Jan 28, 2021 •

edited

Loading

cphyc commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

matthewturk commented Jan 28, 2021

cphyc commented Jan 28, 2021

matthewturk commented Jan 28, 2021 via email

cphyc commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

munkm commented Feb 25, 2021

neutrinoceros commented Feb 25, 2021

munkm commented Feb 25, 2021

neutrinoceros commented Feb 25, 2021

cphyc commented Feb 26, 2021

neutrinoceros left a comment

neutrinoceros Feb 25, 2021

neutrinoceros Feb 25, 2021

		@@ -209,7 +209,9 @@ def get_local_config_file():
		since="4.0.0",

Migration script: migrate with user input #3044

Migration script: migrate with user input #3044

Conversation

cphyc commented Jan 27, 2021 • edited Loading

PR Summary

Xarthisius commented Jan 28, 2021

matthewturk commented Jan 28, 2021 via email

Xarthisius commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

cphyc commented Jan 28, 2021 • edited Loading

neutrinoceros commented Jan 28, 2021 • edited Loading

cphyc commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

matthewturk commented Jan 28, 2021

cphyc commented Jan 28, 2021

matthewturk commented Jan 28, 2021 via email

cphyc commented Jan 28, 2021

neutrinoceros commented Jan 28, 2021

munkm commented Feb 25, 2021

neutrinoceros commented Feb 25, 2021

munkm commented Feb 25, 2021

neutrinoceros commented Feb 25, 2021

cphyc commented Feb 26, 2021

neutrinoceros left a comment

Choose a reason for hiding this comment

neutrinoceros Feb 25, 2021

Choose a reason for hiding this comment

neutrinoceros Feb 25, 2021

Choose a reason for hiding this comment

cphyc commented Jan 27, 2021 •

edited

Loading

cphyc commented Jan 28, 2021 •

edited

Loading

neutrinoceros commented Jan 28, 2021 •

edited

Loading