Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Misc. updates (2.0ish) #2062
Misc. updates (2.0ish) #2062
Changes from 32 commits
1cf6519
8dd77a9
df113ec
4dd5322
fa30e85
27b3007
2e370de
ec5cb83
e7297b5
523af6b
442d325
3e5435a
95965ea
7a59869
a7f35ef
194e1e4
4e1cb47
8ede12f
0930fa4
bf0f04c
59fe782
082d52d
7165723
917b0ab
7dc0596
3cdd408
64bf6b7
dc38b81
919f6fd
0579261
5d38ea0
edf7565
a163a35
25834cc
64b31de
cbd6d62
53ed2d2
d8221e0
19c846a
ff5665d
3e7d40f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use
epochs, learning-rate, batch_siz
above, let's do the same in the example?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is way better 😅. Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a parameter dependency ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This content should be simple enough to avoid additional structure to my mind. Keep it simpler, remove repetitions, move from high level explanation (an example) to the details or/and advanced cases (custom file name).
Also keep in mind, that
dvc params diff
can actually operate on any file now, not even registered indvc.yaml
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be ideal although I'm not sure that sections hurt in this instance (there's several relevant aspects of params so maybe sections are appropriate for a reference doc.)
But 1. all of this content was already there (in fact this PR already makes the text shorter compared to https://dvc.org/doc/command-reference/params and 2. again, since we will move a lot of this info to basic concepts soonish, should we let it be for now?
Good point. It will also operate on parameters insterted to dvc.yaml from
vars
soon... Will address this ⌛ UPDATE: See #2062 (review) below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't see any value in those sections, they have a lot of repetition with the previous introductory section. They don't have any motivation behind them (not clear why do they exist, how do they connect to the other text).
is is addressed already? UPDATE: See #2062 (review) below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in past iterations? I'm not seeing the repetition right now (just 2 mentions of
repro
).They are the references for parameters files and for parameter values, which are not detailed in the Description, but they are linked as part of the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a few concerns (overall comes down to complicating and making things less clear):
Parameters files section
repeats the note to my mind (expands a bit, but it's hard to justify both to my mind)Parameter values
starting from theDVC saves ...
has some general content about the params not only values.They don't have any motivation behind them (not clear why do they exist, how do they connect to the other text).
When I read it I don't understand this structure, why does it exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any repetition I should definitely address (⌛), its kind of a separate issue (it could be there with or without sections).
Good points. OK, I'll removed the headers ⌛
Not sure what you mean by w/o ref. since they're linked to from the main Desc. but indeed there's no intro paragraph giving them some motivation, that's because these are reference docs so we don't have a story in each one. Sections can be pretty useful in this kinds of docs so I'm not sure why we're trying to avoid them in general (your specific points did convince me this time though). For example what about https://dvc.org/doc/command-reference/metrics#supported-file-formats? We have many of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I removed the H3s and addressed the other specific feedback.
I also just realized that the Examples had some problems so I threw that in. PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case we can be specific?
dvc.lock
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I also expanded on this dvc.yaml field since we're encouraging users to avoid dvc run now (so we should better explain all the possible ways to write this manually or generate it).