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

Bump min version of DVC to 2.57.0 (Live share to studio config option) #3976

Merged
merged 6 commits into from
May 28, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented May 25, 2023

1/2 main <- this <- #3987

Relates to one of the checkboxes in #3864

This PR removes our custom logic for sharing experiments live to Studio if the user has a studio.token config value set. We now correctly respect the new studio.offline DVC config value instead of the old dvc.studio.shareExperimentsLive extension config value. The default behaviour is that experiments will be shared live if a token is set (as per the docs).

Needs to be merged before #3965 and they'll go out together.

Demos

Checkbox updating config

Screen.Recording.2023-05-25.at.4.54.15.pm.mov

Workspace run - sharing enabled

Screen.Recording.2023-05-26.at.9.37.07.am.mov

Queue run - sharing enabled

Screen.Recording.2023-05-26.at.9.38.42.am.mov

Workspace run - sharing disabled

Screen.Recording.2023-05-26.at.9.39.23.am.mov

Queue run - sharing disabled

Screen.Recording.2023-05-26.at.9.40.05.am.mov

@mattseddon mattseddon added the product PR that affects product label May 25, 2023
@mattseddon mattseddon self-assigned this May 25, 2023
<a href="https://dvc.org/doc/user-guide/project-structure/configuration#studio">
studio.offline
</a>{' '}
config option.
Copy link
Member Author

@mattseddon mattseddon May 25, 2023

Choose a reason for hiding this comment

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

[F] I have added this small bit of text instead of trying to work out which config file(s) studio.offline is set in and then trying to update the appropriate level(s) of config. Much like updating the studio.token this button will only call config with the --global flag. If users want to save studio.token/studio.offline in project/local configs then they should be capable of managing this themselves. If this does however become an issue I will fix it retroactively.

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've started the process of discussing with the DVC team how we can make this easier to deal with.

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 hint that were setting things globally in the note? For example, The checkbox reflects the global [studio.offline](https://dvc.org/doc/user-guide/project-structure/configuration#studio) config option.

Copy link
Member Author

@mattseddon mattseddon May 26, 2023

Choose a reason for hiding this comment

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

The text in your example isn't necessarily true. If the user has set the option at the project or local level then that option will override what we set at the global level. This is from the docs and hopefully helps to explain a bit further:

image

I have only added the text so that in the (rare) case that the user has set studio.offline=true at the project/local level that the user will have some chance of debugging what is going on.

In order to fully explain this I think I would need to add 2 paragraphs which IMO is not necessary at this stage for onboarding.

This issue does seem to be coming up a lot though so I think I may have to build some representation of dvc config -l --show-origin to add to the page.

@mattseddon mattseddon marked this pull request as ready for review May 26, 2023 00:54
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 26, 2023 00:54
@mattseddon mattseddon requested a review from shcheklein May 26, 2023 00:55
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -0,0 +1,3 @@
.smallFont {
font-size: 0.6rem;
Copy link
Contributor

@julieg18 julieg18 May 26, 2023

Choose a reason for hiding this comment

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

I don't have the best eyesight, but I had to slightly squint to read this 🤣. Should we increase it a bit? Example of 0.7rem:

image

@mattseddon mattseddon enabled auto-merge (squash) May 28, 2023 21:37
@codeclimate
Copy link

codeclimate bot commented May 28, 2023

Code Climate has analyzed commit ea43536 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.1% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit e8b9708 into main May 28, 2023
@mattseddon mattseddon deleted the fix-live-share branch May 28, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants