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

Minor fixes #3319

Merged
merged 6 commits into from
Mar 11, 2022
Merged

Minor fixes #3319

merged 6 commits into from
Mar 11, 2022

Conversation

omesser
Copy link
Contributor

@omesser omesser commented Feb 24, 2022

Minor stuff I hit while glossing over some docs...
Some other thing that bothered me a bit (made me stop and re-read a few times) is the compound noun "pipeline_s_ file(s)" instead of "pipeline file(s)" (pipelines plural), but I ... guess it's valid...? 🤔

@restyled-io restyled-io bot mentioned this pull request Feb 24, 2022
@omesser omesser requested a review from jorgeorpinel February 24, 2022 11:40
Comment on lines 35 to 36
Like with regular experiments, checkpoints can become persistent by
Like with regular experiments, checkpoints can be persisted by
[committing them to Git](#committing-checkpoints-to-git).
Copy link
Contributor

@jorgeorpinel jorgeorpinel Mar 2, 2022

Choose a reason for hiding this comment

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

Yeah we discussed this term and at some point decided not to use it as a verb since it's not the actual meaning of "to persist" in general language.

We do still have this problem in a couple places: mainly in the title of https://dvc.org/doc/user-guide/experiment-management/persisting-experiments and in a hidden section under https://dvc.org/doc/user-guide/experiment-management/running-experiments#the-experiments-queue (minor).

Full discussion: #2960

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel - Thanks for bringing this discussion to my attention.
I was not aware of the rabbit hole 🐇 here 😅 . My edit here was strictly related to the usage as a verb (english) - my 2 cents about that (coincidentally 2 points 😄 ):

  1. re: first point raised in the issue description - Though persist also means "continuing to do something in a determined way" the usage here of "continuing to exist (maybe past an expected/usual time)" is definitely also a valid definition in the general language (See: [1] [2] [3] for example).
  2. Probably more important - This is how it is conventionally used in our domain. "To persist X" means "to commit X to persistency" (of some sorts). So, the way I see it (and the reason I bothered "fixing" it) "checkpoints can become persistent by..." - would sound wrong / clunky to software engineers & people in associated fields (e.g. data engineering) - we have a verb for that. I would argue that familiarity and conciseness win here (even if it wasn't for my point no. 1)

But, on a more constructive note - As per the last comment am I correct to assume it's agreed if I make this change (only here): Like with regular experiments, checkpoints can become persistent by committing them to git -> Like with regular experiments, checkpoints can be committed to git [losing info here, not loving it, but better than now]

I was actually going to suggest following your comment to change the other places you mentioned - namely the section title from Persisting experiments to Committing experiments - but upon further consideration I actually find that problematic 😢 . IMHO "committing" is more of an implementation detail here. If I were to read/skim these docs and compare to other experiment tracking solutions I would look for (search / grep) the terms "persist" and "track" (=what) and not "commit" (=how) - all the more important when it comes to titles!

Copy link
Contributor

@jorgeorpinel jorgeorpinel Mar 10, 2022

Choose a reason for hiding this comment

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

"continuing to exist (maybe past an expected/usual time)" is definitely also a valid definition

Yes but then the phrase should be "checkpoints can persist" which would be unclear. "Become persistent" is a little clunky indeed, but clear: going from temporary to continuous.

Anyway I see you changed it now along the lines of the decisions in #2960 so 👍🏼 but if you want to argue for another phrasing please comment in the issue, we can always revisit all these mentions.

UPDATE: I moved some of your comments to the issue now. We can discuss there.

@jorgeorpinel
Copy link
Contributor

p.s. sorry for the delay @omesser . Assigned you for visibility in https://github.com/orgs/iterative/projects/284.

@omesser
Copy link
Contributor Author

omesser commented Mar 6, 2022

Hey @jorgeorpinel,
Pushed some changes and answered some comments - please do another pass when you get a chance

Copy link
Contributor

@iesahin iesahin left a comment

Choose a reason for hiding this comment

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

Other than a small point regarding checkpoints.md, looks good to go. Thank you @omesser

@jorgeorpinel jorgeorpinel merged commit 4ea0908 into iterative:master Mar 11, 2022
@jorgeorpinel
Copy link
Contributor

Thanks @omesser !

@casperdcl
Copy link
Contributor

Totally reminds me of my first docs PR here. Some things just feel unignorable @omesser :)

iesahin pushed a commit that referenced this pull request Apr 11, 2022
* initial

* pipelines....files?

* Apply restyle changes

* Another minor find

* CR fixes pass

* committed
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.

4 participants