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
ref:
exp init
improvements #3071ref:
exp init
improvements #3071Changes from 1 commit
153d526
38099d3
0509a51
97592c7
e70ea6d
a97e611
8b89b8f
2273431
3d91656
69c8082
8f21601
be7a5af
600e83c
6e3d2fa
42aa486
5e09813
56f4a46
fac16bd
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.
Somewhere in the description, it might be useful to explain that
dvc exp init
by default expects that input data, parameters, and source code paths exist before running an experiment, and that the command is expected to generate models, metrics, and plots.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.
That's more or a problem for
exp run
though, @dberenbaum (already linked form the--run
option). But should we state thatexp run
(andrepro
for that matter) expect that the stage definition and code are good? Hopefully its evident.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.
Well that's the point of
exp init
, right? Better to have users understand what's needed up front than to have them runexp init
only to fail onexp run
.Doesn't need to be part of this PR. It could also be handled by some of the suggested changes to the core command rather than the docs.
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'm not sure. If users need to read that on the cmd ref., is
exp init
serving as a self-explanatory command? I think those notes should be in the command's output fist (and added to this doc as a result of that product update).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.
side note ... for the sake of my education: why is it required?
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.
To parse the parameters and put them into
dvc.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.
Yeah but even if you don't have params an empty file is still required.
Related to iterative/dvc#6446 (comment)
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.
It's also related to iterative/dvc#6605. The
params
section ofdvc.yaml
currently expects keys from a file (see below), not a filename itself, so all the parameters need to be defined before creating the stage. If we want to stop requiring this file at the time ofdvc exp init
, we need to changeparams
to accept filenames.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.
What about generating a dvc.yaml without
params
if there's no parmas.yaml file? This is a core discussion though! Moved to iterative/dvc#6446 (comment)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.
Sorry, I misunderstood you, and now I think it's actually more of a docs issue 🤔 . It is possible to use
dvc exp init
withoutparams
. You can either use--explicit
and not provide--params
, or you can usedvc exp init -i
and typen
at the params prompt.The note here is to indicate that if you do use either the default or some other
params
path, then that path must exist and be parseable to extract the parameters for the stage. So the current note probably needs to be reworded.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.
UPDATE: Looks like the discussion about empty params.yaml files is in a few core repo tickets now (e.g. iterative/dvc#7138) so I won't keep discussing here (still a bit confused though).
Clarified in 2273431.
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.
BTW a question I have is whether to keep this section or move
command
under Options like we did fortargets
in https://dvc.org/doc/command-reference/repro#options. I personally like the section more but I remember we discussed it (cc @shcheklein) this and using Options was picked, so for consistency I'd move this under Options as well.