Skip to content

Conversation

@daavoo
Copy link
Contributor

@daavoo daavoo commented Jan 26, 2022

Added a hacky script and plugin for updating tables in order to reflect changes to the core dvc exp show.

What I do locally to update all tables to reflect the latest dvc changes is:

python scripts/create_dvctables.py plugins/gatsby-remark-dvctable-filler/tables.js

And update scripts/create_dvctables.py if I need to add a new table.

@daavoo daavoo requested a review from julieg18 January 26, 2022 16:34
@shcheklein shcheklein temporarily deployed to dvc-org-create-exp-show-dscfsv January 26, 2022 16:34 Inactive
@daavoo daavoo requested a review from iesahin January 26, 2022 16:38
@shcheklein shcheklein temporarily deployed to dvc-org-create-exp-show-dscfsv January 26, 2022 16:38 Inactive
├── 6d13f33 [cnn-64] Sep 09, 2021 0.23385 0.9153 10 64
├── 69503c6 [cnn-128] Sep 09, 2021 0.23243 0.916 10 128
───────────────────────────────────────────────────────────────────────────────────────────
$dvc-experiments-exp-show
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iesahin I have only updated this one because the others are using the old --include/--exclude arguments (leftover from #3112 )

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, if we're to check in this line to Git, we'll need to run the script to populate the tables, right? My "creating a dependency" comment is about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual table that replaces each of these placeholders (${X}) is also committed in Git (https://github.com/iterative/dvc.org/blob/083f0655b98bfb193a1fd96e00a4cb256328af9e/plugins/gatsby-remark-dvctable-filler/tables.js).

There is no need to run the script, yarn develop will replace the placeholder with the corresponding table content.

@shcheklein shcheklein temporarily deployed to dvc-org-create-exp-show-dscfsv January 26, 2022 17:18 Inactive
@daavoo daavoo mentioned this pull request Jan 26, 2022
@julieg18
Copy link
Contributor

Added a hacky script and plugin for updating tables in order to reflect changes to the core dvc exp show.

What changes to the core dvc exp show are you refering to?

@daavoo
Copy link
Contributor Author

daavoo commented Jan 27, 2022

What changes to the core dvc exp show are you refering to?

Any change that updates the output or the style of the table generated by dvc exp show.

For example, treeverse/dvc#7089 adds the new type of columns affecting practically all tables.

If I had to send the doc updates without the script and plugin added in this P.R., I would have to manually review the existing tables and, for each one:

  • try to find out a dvc command and corresponding example project to replicate the table.
  • Setup the example project.
  • Run the command locally.
  • Copy-paste the output to the code block.

With this P.R. I just run the create_dvctables.py script

@iesahin
Copy link
Contributor

iesahin commented Jan 27, 2022

It looks this creates a dependency to DVC in dvc.org builds. This was something we were avoiding. WDYT @rogermparent @shcheklein ?

I have a more general markdown-code-runner which basically runs the commands and replaces the code sections with their outputs.

However, I'm not fully comfortable to depend DVC for the site.

@daavoo
Copy link
Contributor Author

daavoo commented Jan 27, 2022

It looks this creates a dependency to DVC in dvc.org builds. This was something we were avoiding. WDYT @rogermparent @shcheklein ?

Given that this doesn't run automatically, I don't fully understand how this creates a dependency if I would be basically doing the same as the script and manually copy pasting to the md files.

I have a more general markdown-code-runner which basically runs the commands and replaces the code sections with their outputs.

I have seen that (and like 👍 ) but didn't work exactly as I intended for the tables so I added the plugin

@rogermparent
Copy link
Contributor

Being able to generate code blocks for table output is a good idea, and I think the loose dependency on dvc isn't a dealbreaker, but this PR does exceed my threshold for acceptable hackiness a bit- particularly how new tables have to be added in the middle of a Python source file and not even its own dedicated config file.

I think the website and docs teams can collaborate to add a but more polished implementation of this feature if we deem it important enough.

@julieg18
Copy link
Contributor

Being able to generate code blocks for table output is a good idea, and I think the loose dependency on dvc isn't a dealbreaker, but this PR does exceed my threshold for acceptable hackiness a bit- particularly how new tables have to be added in the middle of a Python source file and not even its own dedicated config file.

I think the website and docs teams can collaborate to add a but more polished implementation of this feature if we deem it important enough.

Agreeing with @rogermparent, this might be too hacky, even for a first iteration. 🤔 Also if we decide to do something like this, whether by merging this pr or coming up with a different implementation later, we'll probably need to document how it works and what you need to run the script.

@daavoo
Copy link
Contributor Author

daavoo commented Jan 27, 2022

@rogermparent , @julieg18 I totally agree with you.

I just wanted to try to push a little bit the discussion about automating the generation of some parts of the docs.

In the case of the exp show tables, the time I had already spent manually editing tables started to feel a little cumbersome, and decided to invest minimum viable time on automating it.

However, I don't want this hacky stuff to generate additional unplanned work for the websites team.

Maybe we can push back this P.R. and bring back the discussion in #2770 (I will continue using hacky script locally but sent the final rendered results on P.R. so it would be just markdown updates)

@daavoo daavoo closed this Jan 27, 2022
@shcheklein
Copy link
Contributor

I like the idea and I really appreciate the amount of effort @daavoo put into this! Let's try to collaborate and get to the state we all want it to be if agree on the approach.

Just to clarify the benefits (if we fix all these small things here and there) of the approach:

  • we can share the same table across multiple docs - less points to update. How many example like this we have?
  • we can quickly go into a single file and update it. We can keep some information about how this table is generated to update it later.
  • we can run a script now and generate the new version

regarding the last point - how does it actually happen? do we run an actual DVC command in the repo or we can just copy paste the output? any other things I'm missing?

cons that I see so far:

  • files become less human readable/editable with this special syntax

anything else?

@julieg18
Copy link
Contributor

Maybe we can push back this P.R. and bring back the discussion in #2770 (I will continue using hacky script locally but sent the final rendered results on P.R. so it would be just markdown updates)

We should definitely continue to discuss and work on this! #2770 seems to be more focused on autogenerating the markdown for the Options/Synopsis sections. @daavoo, should we open a separate issue for discussing autogenerating tables or discuss both from there?

@daavoo daavoo deleted the create-exp-show-tables branch July 29, 2022 09:21
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.

5 participants