Skip to content

Conversation

@daavoo
Copy link
Contributor

@daavoo daavoo commented Jan 26, 2022

P.R. for treeverse/dvc#7089

Closes #3211

@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 26, 2022 17:31 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 26, 2022 17:39 Inactive
@daavoo daavoo force-pushed the dvc-exp-show-deps branch from 3447147 to 2d73993 Compare January 27, 2022 21:43
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 27, 2022 21:44 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 27, 2022 21:46 Inactive
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 28, 2022

Looks like a change with a big impact on existing docs... @daavoo you can scope these to cmd ref only 🙂

We can make issues to update Get Started and Guides later if needed; It may involve discussing so it's OK if that takes longer and falls on docs team. But if you think the work so far on those should be reviewed, lmk (we may just need to extract that to 1+ separate PRs).

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 28, 2022

And another scope-related Q: is it too much to cover treeverse/dvc#7089 as well as #3202 and #3211 in a single PR? Maybe the latter 2 should be worked on first/separately.

@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 28, 2022 10:54 Inactive
@daavoo daavoo marked this pull request as ready for review January 28, 2022 12:00
@daavoo daavoo requested review from dberenbaum and iesahin January 28, 2022 12:00
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 28, 2022 12:00 Inactive
@daavoo
Copy link
Contributor Author

daavoo commented Jan 28, 2022

We can make issues to update Get Started and Guides later if needed; It may involve discussing so it's OK if that takes longer and falls on docs team. But if you think the work so far on those should be reviewed, lmk (we may just need to extract that to 1+ separate PRs).

I' ve already included updates for Get Started and 1 of the tables in the user guide. I'll open an issue for the user guide updates.

And another scope-related Q: is it too much to cover treeverse/dvc#7089 as well as #3202 and #3211 in a single PR?

Extracted #3237

and params from the entire project. The `--only-changed`, `--drop`, `--keep`,
and other [options](#options) can determine which ones should be displayed.
By default, the printed experiments table will include columns for all
dependencies, metrics and params from the entire project. The `--only-changed`,
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, should we order in the same way they appear (metrics, params, deps) throughout the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the order.

I have also noticed that params and parameters are used arbitrarily.

Should we consolidate params / deps or the longer version? cc @jorgeorpinel

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer the longer versions thinking they are more likely to be searched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced all occurrences with long version

@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl January 31, 2022 11:15 Inactive
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.

Looks good to me but I'll wait for @jorgeorpinel 's review before merge.

@jorgeorpinel
Copy link
Contributor

I'll wait for @jorgeorpinel 's review before merge

You and @dberenbaum take this one please. I got a few other ones. My initial concerns were addressed. Thanks

@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl February 2, 2022 17:24 Inactive
@daavoo daavoo requested a review from dberenbaum February 2, 2022 17:36
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl February 2, 2022 17:36 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl February 2, 2022 17:38 Inactive
Co-authored-by: Dave Berenbaum <dave@iterative.ai>
@restyled-io restyled-io bot mentioned this pull request Feb 3, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl February 3, 2022 11:14 Inactive
Co-authored-by: Restyled.io <commits@restyled.io>
@shcheklein shcheklein temporarily deployed to dvc-org-dvc-exp-show-de-2k72nl February 3, 2022 11:15 Inactive
@iesahin iesahin merged commit 5460bd9 into master Feb 3, 2022
@iesahin iesahin deleted the dvc-exp-show-deps branch February 3, 2022 14:31
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* dvctable: Add `violet` for `dep` columns.

* Add dep columns in `exp show` ref.

* Mention dependencies in start and user-guide.

* Update table

* Update tables in get started.

* Mention column order

* Order metrics, params and dependencies.

* Replace params with parameters

* Remove dvctables

* Apply suggestions from code review

Co-authored-by: Dave Berenbaum <dave@iterative.ai>

* Restyled by prettier (#3254)

Co-authored-by: Restyled.io <commits@restyled.io>

Co-authored-by: Dave Berenbaum <dave@iterative.ai>
Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
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.

Document the color and order of columns in exp show table

5 participants