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

sharing exp: document studio.token #4470

Merged
merged 1 commit into from
Apr 20, 2023
Merged

sharing exp: document studio.token #4470

merged 1 commit into from
Apr 20, 2023

Conversation

skshetry
Copy link
Member

Related #4440.

I did not add any visuals for exp push as I did not see if it fit into command reference. Maybe that should be accommodated in the guide or tutorials.

@skshetry skshetry requested review from dberenbaum and daavoo April 17, 2023 03:17
@skshetry skshetry self-assigned this Apr 17, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-studio-tokens-xmoqgzjb April 17, 2023 03:20 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

Link Check Report

There were no links to check!

@dberenbaum
Copy link
Contributor

@daavoo @skshetry It feels like we need to clean up the env vars and make them more consistent between live sharing and exp push. I think it's the last blocker for general usage of the exp sharing features. WDYT? Can one of you take it?

@skshetry
Copy link
Member Author

I'm a bit hesitant to add env vars for every config. :(

@shcheklein shcheklein temporarily deployed to dvc-org-studio-tokens-xmoqgzjb April 18, 2023 07:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-studio-tokens-xmoqgzjb April 18, 2023 07:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-studio-tokens-xmoqgzjb April 18, 2023 07:47 Inactive
Comment on lines +15 to +19
Iterative Studio uses access tokens to authorize DVC and [DVCLive] to send
experiments and live updates to the metrics and plots. The access token must be
present in any request that sends data to the Iterative Studio ingestion
endpoint. Requests with missing or incorrect access tokens are rejected with an
appropriate HTTP error code and error message.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the access token is used by dvc too, I think we need to rewrite this page or extract it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point.
Probably outside the scope of the P.R. and better to be discussed in #4440

I would perhaps not touch this introduction for now and leave the page to be only about live updates

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should at least mention "to send experiments" here as we are redirecting here for setup instructions. Let me know what you think (that's the only change here btw).

Copy link
Contributor

@daavoo daavoo Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should at least mention "to send experiments" here as we are redirecting here for setup instructions. Let me know what you think (that's the only change here btw).

I am ok with merging this as it is.

we are redirecting here for setup instructions

As you mentioned, what we probably need to do is to extract the info, for example having something like a connect to Studio page that doesn't get into the downstream use case (live updates, exp push, etc.), and use that page for cross reference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me for now. I'm a little hesitant to say we need another page just for how to setup an access token. Feels like it creates more friction for live sharing. @tapadipti PTAL and add any thoughts you have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tapadipti Do we have `any of the other env variables mentioned in this guide? For customers who have on-prem Studio, how do they know how to configure live metrics and plots?

Copy link
Member Author

@skshetry skshetry Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant to say we need another page just for how to setup an access token.

yeah, I think it's simple enough that we don't need to have a separate page. If we need it, it might be a UX problem. (For 1, I get confused between Settings page and Profile page in Viewer).

@daavoo
Copy link
Contributor

daavoo commented Apr 18, 2023

@daavoo @skshetry It feels like we need to clean up the env vars and make them more consistent between live sharing and exp push. I think it's the last blocker for general usage of the exp sharing features. WDYT? Can one of you take it?

@dberenbaum Do you also mean that DVCLive should check for dvc config?

@skshetry
Copy link
Member Author

@daavoo, I understood it to just envvars. See #4470 (comment).

Comment on lines 52 to 53
view experiments. Check
[this guide on how to setup an access token](/doc/studio/user-guide/projects-and-experiments/live-metrics-and-plots#set-up-an-access-token).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
view experiments. Check
[this guide on how to setup an access token](/doc/studio/user-guide/projects-and-experiments/live-metrics-and-plots#set-up-an-access-token).
view experiments. [Get the token](https://studio.iterative.ai/user/_/profile?section=accessToken) or check
[this guide on how to create and manage an access token](/doc/studio/user-guide/projects-and-experiments/live-metrics-and-plots#create-and-manage-access-token).

Rather go through more docs, I would suggest we first try to send people directly to a link to get their token. If they have an on-prem setup or special needs, we can also link to the guide. I'd rather link to the Create and manage access token of the guide since it gets directly to how to get the token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention the Studio URL config also here? Ideally, it should be in the linked guide IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that underscore URL stable and guaranteed to work? And not the side effect of frameworks that they use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that above the url does not handle if you are logged out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that underscore URL stable and guaranteed to work? And not the side effect of frameworks that they use?

I got it from VS Code, who is using it to direct users to the token. @iterative/studio can answer whether it's stable I guess.

Note that above the url does not handle if you are logged out

You mean it doesn't redirect after login?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou @mvshmakov for confirmation. One more question, is it possible to redirect after login?

Copy link
Contributor

@mvshmakov mvshmakov Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question, is it possible to redirect after login?

Yes, with conventional ?next={url}. Better to double-check before writing it down.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant if it's possible to handle that on Studio side without doing anything here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. Is this a question about whether it is possible to make this link work even if the user is logged out? E.g., if the user is not logged in, to redirect the user to the login page and after a login redirect to the URL which was passed before?

We have to consider a few edge cases here, and now it is not supported. If this is what you wanted, please create an issue in the Studio repo. If not, could you please elaborate on the question a bit more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a question about whether it is possible to make this link work even if the user is logged out? E.g., if the user is not logged in, to redirect the user to the login page and after a login redirect to the URL which was passed before?

yes, that's what I meant. Will create an issue. Thanks.

@dberenbaum dberenbaum requested a review from tapadipti April 18, 2023 14:40
@dberenbaum
Copy link
Contributor

@skshetry Left a couple suggestions to directly link to the Studio profile page to get the token. I think not some users just want to know where to find the token. If you're okay with those, I think it's ready to merge and we can follow up on other questions about how to reorganize the Studio docs.

@shcheklein shcheklein temporarily deployed to dvc-org-studio-tokens-xmoqgzjb April 19, 2023 07:30 Inactive
@skshetry skshetry merged commit bd339fb into main Apr 20, 2023
@skshetry skshetry deleted the studio-tokens branch April 20, 2023 09:53
@dberenbaum
Copy link
Contributor

Thanks @skshetry! Sorry, didn't get a chance to approve it after your latest changes, but looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants