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: document
dvc queue
#3715ref: document
dvc queue
#3715Changes from 26 commits
4e68869
dbe354c
fb61d31
817afb7
a26ee1d
e5feaf4
8a4b3d8
75e21a3
01ee250
ea22d10
89041fa
f5fef4d
29760dc
533f175
6677c18
f337cad
ecd46d4
d230f9a
7ae6413
8f1681b
09a23bc
7df232e
c87cc2a
6a17f18
7264c94
e571871
68d34e4
8fd6a29
5d556a1
45183e7
f6061dd
ed2e412
e1930b4
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.
Maybe this should be under
--run-all
? Or split into 2 admonitions, even if they're similar.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.
@pmrowla What do you think about moving or splitting this one?
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 don't really think it's necessary to repeat this block after 2 consecutive options -
run-all
andjobs
will appear one after another and both options + the info block fit into a single screen on most devicesThere 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.
True. But this is much more about
--run-all
than about--jobs
so I moved it up in 26d28e8.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 about this terminology: "experiment task queue"
Why not just "experiment queue" ? Are "tasks" important enough to differentiate from experiments? (At first glance it sounds like an implementation detail)
WDYT @dberenbaum ? Could impact strings in the core codebase.
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 think it's important to differentiate between "experiment" vs "queue task" due to commands where there's overlap (like
exp remove
vsqueue remove
).exp remove
removes DVC experiment data, meaning experiment git refs and their associated DVC cache data.queue remove
removes task queue entry data, meaning the queue entry itself, and associated queue-related artifacts (i.e. logs)The current plan is also to emphasize this in other commands.
exp show
will be modified so that by default it does not display queued or failed experiments by default (so the default view only shows things that can be removed withexp remove
). There will still be optional flags for displaying queued/failed exps in the table (since it is useful to be able to see the params/deps for them).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.
@jorgeorpinel Maintaining two separate concepts is important, but feel free to suggest more useful terminology 🙏 .
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 see there's some rationale and justification for the special terminology. I still think "task" is an implementation detail and could be avoided...
I'm thinking that it should be clear that
exp
commands deal with experiment data whilequeue
commands deal with experiment queue entries (I mean that was the whole point of separating the commands I think 🙂) so term "experiment" can be used interchangeably in many places. Where disambiguation or emphasis are needed, we can use descriptive wording like "entry from the queue", "worker logs", etc.But it's something we can follow-up on later if needed (still unsure about the release timeline we're looking at here).
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 is in main now and should be in the next DVC release.
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.
Everyone raises good points here. Agreed that
dvc queue
should be used infrequently. No functionality was removed fromdvc exp
here. For example, you can remove queued experiments without usingdvc queue
:In this case,
dvc exp remove
anddvc queue remove
are aliases, butdvc queue remove
is needed to do things like drop successful experiments fromdvc queue status
andlogs
(and it seemed better to allow flexibility to remove any task from the queue as long as we have the command).When the
dvc queue
commands are needed, it often is helpful to distinguish between the experiment and the queue task. Examples include completed experiments where you might want to clear them and their logs from the queue but keep the results, or checkpoints where there are multiple experiment rows for a single queue task (note that you can use the finished experiment names in thedvc queue
commands, likedvc queue logs exp-7002e
).Ideas to further improve:
dvc exp
regarding the queue, we should add it where possible (for example, I noticedvc exp remove
doesn't work for failed experiments, so I'll open an issue).dvc exp remove --queued
).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.
My only specific suggestion for now is to avoid term "task". Be descriptive instead e.g. "queued experiment", "experiment from queue", even "entry from exps queue" if needed.
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.
@jorgeorpinel if
queue
is considered a low level building block (and eventually can do things besides experiments for example), it's fine to usetask
here to my mind.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 strongly opposed but it seems unnecessary to me: the whole task management aspect of this is an implementation detail (ultimately irrelevant for users).
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.
But "queue" is the name of the command. Hard to avoid that term.
p.s. term "worker process" also seems redundant and too deep. We already have "jobs" for the option name, I'd stick to that.
In some places we even have the special combo "task queue worker process" -- words have lost all meaning 😋
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.
Let's provide just a bit more context instead like "You can use
exp run --queue
to queue experiments and then..." instead of repeating the definition of the command (already in the top of the page). ThanksThere 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.
@pmrowla What do you think? Could you add some phrasing like this?
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.
Probably too late to discuss now but just adding a note for a possible design follow-up:
queue remove --queued
sounds redundant. Maybe--unprocessed/waiting
or something like that?Also,
queue remove --success
->--successful
, I think.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 one was just an idea for you @dberenbaum, no follow-up needed in #3894.
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 a good point but not a high priority for me. Feel free to open an issue and maybe we can change it or make
--queued
a hidden alias when we have time.