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

[WIP] cmd-ref: document exp init #3015

Merged
merged 9 commits into from
Dec 7, 2021
Merged

[WIP] cmd-ref: document exp init #3015

merged 9 commits into from
Dec 7, 2021

Conversation

skshetry
Copy link
Member

Fixes #2974.

@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 11, 2021 13:26 Inactive
@skshetry skshetry self-assigned this Nov 11, 2021
@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 11, 2021 13:26 Inactive
@dberenbaum
Copy link
Collaborator

@jorgeorpinel What sections do you think are needed for the cmd-ref? We will definitely want a user guide for this, but since we are trying to keep the cmd-ref more narrow, maybe we can eliminate some of these sections.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

GH seems to have a bug where I can't leave single comments, only review comments... Sorry for the noise.

@@ -37,6 +38,7 @@ positional arguments:
push Push a local experiment to a Git remote.
pull Pull an experiment from a Git remote.
remove Remove local experiments.
init Initialize experiments.
Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 11, 2021

Choose a reason for hiding this comment

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

Uninformative and sounds like a requirement (like dvc init). How can we better describe what this does for the user? (I need to read the rest of the changes before I can suggest something.)

This comment was marked as resolved.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Nov 11, 2021

Choose a reason for hiding this comment

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

For the help output maybe a short form of a specific desc.

Codify a project variation and run it as an experiment

p.s. and re "sounds like a requirement (like dvc init)" - can we still reconsider the name? 😅
Maybe dvc exp new

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

More noise... Ignore these.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

On the command description

@@ -37,6 +38,7 @@ positional arguments:
push Push a local experiment to a Git remote.
pull Pull an experiment from a Git remote.
remove Remove local experiments.
init Initialize experiments.

This comment was marked as resolved.

content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
Comment on lines +31 to +37
### The `command` argument

The `command` argument is optional, if you are using `--interactive` mode. The
`command` sent to `dvc exp init` can be anything your terminal would accept and
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 put this under Options like for targets in https://dvc.org/doc/command-reference/repro#options (and other places I think) ? That was an initiative of @skshetry actually 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it as it is for now, as we also need to do something for stage add/run?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why keep it as a section here when we moved it in repro ? We had lots of good reasons (which you proposed), what's different?

do something for stage add/run

Out of scope but we can create an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's better to do it at the same time for all commands. I am going with what we have right now, let's handle it later?

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 10, 2021

Choose a reason for hiding this comment

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

p.s. we don't always make changes for all commands at once, we can take them as they come. BTW I actually like a section better than having it under Options, I'm just confused since we discussed this a lot for repro and you (and Ivan) strongly argued for putting it under Options I think 🤷 now it's unclear which one is inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's better to do it at the same time for all commands. I am going with what we have right now, let's handle it later?

#3071 (review)

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Noise

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Couple small fixes (committing...)

content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 11, 2021 19:33 Inactive
Comment on lines 125 to 139
## Setting up custom config/Sharing configs

## Non-interactive mode

## Guided/Interactive mode

## Examples

#### Default stage

#### Dvclive stage
Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgeorpinel What sections do you think are needed for the cmd-ref? We will definitely want a user guide for this, but since we are trying to keep the cmd-ref more narrow, maybe we can eliminate some of these sections.

@dberenbaum yes we can keep it much leaner now and leave much of the details for a guide. But it's good to have a proposed structure in advance so thanks @skshetry

For now each proposed section can be a short paragraph in the description without any subtitle structure. Let's just mention the features without explaining exactly how they work as the guide will do that. For now just awareness + the help output will have to suffice I think.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Noise

content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
Comment on lines 24 to 25
data, parameters, source code, models, metrics and plots, which can be
customized through config.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
data, parameters, source code, models, metrics and plots, which can be
customized through config.
data, parameters, source code, models, metrics and plots. The defaults
can be customized through config to reuse across projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you customize "through config"? dvc config? Config files? Which one(s). Thanks

Copy link
Member Author

@skshetry skshetry Nov 22, 2021

Choose a reason for hiding this comment

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

Both? But I am not sure we need to specify that, as dvc config operates on config files, it's just a helper for that.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 1, 2021

Choose a reason for hiding this comment

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

OK. My point is that need to be a bit more specific and/or link to what that config looks like (how it's setup). Also, separate Q:

to reuse across projects

Does that mean using user/system config? I think all these details should be somewhere else, maybe a separate subsection for now and link there from here? Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I have a section below with the title Setting up custom config/Sharing configs. Just need to hyperlink it, and fill that section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @skshetry is there some more info on exp init configuration files in the wiki or somewhere? Did anything change in the config command? Thanks

content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
content/docs/command-reference/exp/init.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 27, 2021 00:30 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 29, 2021 23:00 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 November 29, 2021 23:00 Inactive
@iterative iterative deleted a comment from dberenbaum Dec 1, 2021
@jorgeorpinel jorgeorpinel added the p1-important Active priorities to deal within next sprints label Dec 1, 2021
@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 December 2, 2021 14:12 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 December 2, 2021 14:48 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 December 2, 2021 16:53 Inactive
Comment on lines 150 to 153
## Non-interactive mode

## Guided/Interactive mode

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 don't really want to show all the prompts. And I think most of the information are already mentioned elsewhere, so I am thinking of leaving these fields.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 10, 2021

Choose a reason for hiding this comment

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

Good call. If we ever make a detailed guide we can include all prompts but it's probably unnecessary (and a lot to maintain) either way.

@dberenbaum
Copy link
Collaborator

What's the status on this @skshetry @jorgeorpinel? I can take over if needed. This should get merged today.

@shcheklein shcheklein temporarily deployed to dvc-org-exp-init-doc-dd9iqakf8 December 6, 2021 20:52 Inactive
Copy link
Collaborator

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Merging this since we need some reference available.

@dberenbaum dberenbaum merged commit 03db469 into master Dec 7, 2021
@dberenbaum dberenbaum deleted the exp-init-doc branch December 7, 2021 11:43
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* cmd-ref: document exp init

* Restyle [WIP] cmd-ref: document exp init (#3016)

Co-authored-by: Restyled.io <[email protected]>

* Apply suggestions from code review

* Restyled by prettier (#3018)

Co-authored-by: Restyled.io <[email protected]>

* Update content/docs/command-reference/exp/index.md

* Restyled by prettier (#3048)

Co-authored-by: Restyled.io <[email protected]>

* updates

* document config

* cmd-ref exp init: updates for initial merge

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Jorge Orpinel <[email protected]>
Co-authored-by: dberenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-important Active priorities to deal within next sprints
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ref: Document dvc exp init
4 participants