Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Studio onboarding screen and workflow #3864

Closed
2 of 3 tasks
shcheklein opened this issue May 10, 2023 · 14 comments
Closed
2 of 3 tasks

Simplify Studio onboarding screen and workflow #3864

shcheklein opened this issue May 10, 2023 · 14 comments
Assignees
Labels
A: onboarding Improving and simplifying users happy path. How do we get them have value asap? priority-p1 Regular product backlog

Comments

@shcheklein
Copy link
Member

shcheklein commented May 10, 2023

Two things that I found recently trying it:

Related #3434

  • I think we should have a single button Get Token (Studio can redirect to the right page, and even if doesn't now I think we should be fine for now). @iterative/studio-frontend can Studio redirect now? What is the complexity of this? To clarify - if I sign up I want it to immediately open the page with my token. We next need to think about also creating a project automatically or semi-automatically since we know the repo URL potentially and can pass it in the studio URL.
  • If an ENV variable with a token detect we need to at least say something about it. ENV var in Codespaces is enough to log metrics actually, and some options on this page can contradict with it. E.g. should we show that checkbox enabled it token ENV var is found and if ppl disable it then unset the env?
  • After Live metrics config dvc#9433, live sharing will be enabled by default when the studio token is saved to the dvc config, although it can be disabled with studio.offline = true. To comply with this, we need to make the "Share new experiments live" checkbox correspond to the studio.offline config value and stop setting the environment variable.
Screenshot 2023-05-10 at 10 48 22 AM
@shcheklein shcheklein added priority-p1 Regular product backlog A: onboarding Improving and simplifying users happy path. How do we get them have value asap? labels May 10, 2023
@mattseddon
Copy link
Member

The first item is covered by the last item in #3574.

Drop the sign-in button from Studio setup page (#3574 (comment) - related to https://github.com/iterative/studio/issues/5715)

The Studio issue for frontend redirects is https://github.com/iterative/studio/issues/5715.

We can remove the original action item from #3574 if you like.

@shcheklein
Copy link
Member Author

@mattseddon thanks, yep. Dropped it in that ticket (making it closer to close that one).

@dberenbaum
Copy link
Contributor

I think we should have a single button Get Token (Studio can redirect to the right page, and even if doesn't now I think we should be fine for now).

👍

@iterative/studio-frontend can Studio redirect now? What is the complexity of this? To clarify - if I sign up I want it to immediately open the page with my token. We next need to think about also creating a project automatically or semi-automatically since we know the repo URL potentially and can pass it in the studio URL.

Should we first focus on the part about creating a project? If it's about onboarding, I think first priority needs to be to create the project and token only matters after that.

@dberenbaum
Copy link
Contributor

From #3898:

After iterative/dvc#9433, live sharing will be enabled by default when the studio token is saved to the dvc config, although it can be disabled with studio.offline = true. To comply with this, we need to make the "Share new experiments live" checkbox correspond to the studio.offline config value and stop setting the environment variable.

@mattseddon
Copy link
Member

ENV var in Codespaces is enough to log metrics actually, and some options on this page can contradict with it. E.g. should we show that checkbox enabled it token ENV var is found and if ppl disable it then unset the env?

@shcheklein do you mean you can set an environment variable when starting codespaces which will log metrics to Studio if any experiments are run through the extension?

@shcheklein
Copy link
Member Author

@shcheklein do you mean you can set an environment variable when starting codespaces which will log metrics to Studio if any experiments are run through the extension?

Yes, exactly.

@mattseddon
Copy link
Member

mattseddon commented Jun 6, 2023

@shcheklein do you mean you can set an environment variable when starting codespaces which will log metrics to Studio if any experiments are run through the extension?

Yes, exactly.

This seems like a strange/advanced use case now that the token is managed via a user's DVC config(s). I think the smartest thing to do would be to block it off and remove STUDIO_TOKEN DVC_STUDIO_TOKEN from the environment variables we pass to a new process.

@shcheklein
Copy link
Member Author

This seems like a strange/advanced use case now that the token is managed via a user's DVC config(s).

Yes, agreed.

I think the smartest thing to do would be to block it off and remove STUDIO_TOKEN from the environment variables we pass to a new process.

Hmm, sounds like too magical and a bit too intrusive to me. How about we show a message if it's detected (in the Setup screen) or as a notification?

@mattseddon
Copy link
Member

Hmm, sounds like too magical and a bit too intrusive to me. How about we show a message if it's detected (in the Setup screen) or as a notification?

There is only a passing reference in the docs to using the token in the way that we are discussing:

I feel like it is more magical that it actually works and that we don't need to come up with a workaround to cater for this edge case.

@shcheklein
Copy link
Member Author

It's still a valid setup (otherwise we would have deprecated it). I agree though that it's a minor / advanced and we can deprioritize it. (I would not ignore / remove the env variable though, let's user do what they want).

@mattseddon mattseddon added the blocked Issue or pull request blocked due to other dependencies or issues label Jun 13, 2023
@mattseddon mattseddon removed their assignment Aug 9, 2023
@julieg18
Copy link
Contributor

julieg18 commented Oct 24, 2023

Studio has implemented a way to get a token with Device Authentication (see https://github.com/iterative/studio/issues/5158#issuecomment-1772610546). I'll start looking into using the method in VSCode.

@julieg18 julieg18 removed the blocked Issue or pull request blocked due to other dependencies or issues label Oct 24, 2023
@julieg18
Copy link
Contributor

julieg18 commented Oct 30, 2023

Researched how other extensions handled authentication like this and posted findings in Studio (https://github.com/iterative/studio/issues/5158#issuecomment-1783088262). TLDR is that with the current flow, two issues I noticed were that we need to repeatedly poll a studio endpoint for a token and the user doesn't get redirected back to vscode after authentication.

I also created a quick demo of how things would work with the current Studio flow (#4931
):

Screen.Recording.2023-10-30.at.6.30.02.PM.mov

We could either keep working on this and push a first iteration with the current Studio flow, or hold off until we have another iteration on Studio's auth flow. Personally, I think the current flow is good enough to release, but what does everyone else think? cc @shcheklein

Either way, I'll keep working on the pr, since a good chunk of the logic should stay the same.

@julieg18
Copy link
Contributor

julieg18 commented Nov 14, 2023

With #4931 merged, we now use studio's auth flow in VSCode! What's left:

@julieg18
Copy link
Contributor

julieg18 commented Dec 6, 2023

Going to close this since all tasks have been done, minus this:

If an ENV variable with a token detect we need to at least say something about it. ENV var in Codespaces is enough to log metrics actually, and some options on this page can contradict with it. E.g. should we show that checkbox enabled it token ENV var is found and if ppl disable it then unset the env?

As discussed synchronously, it is extremely difficult for the extension not detect ENV variables.

@julieg18 julieg18 closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: onboarding Improving and simplifying users happy path. How do we get them have value asap? priority-p1 Regular product backlog
Projects
None yet
Development

No branches or pull requests

4 participants