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

Basic Concepts Section for the Documentation #2177

Closed
wants to merge 39 commits into from

Conversation

iesahin
Copy link
Contributor

@iesahin iesahin commented Feb 11, 2021

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Emre Şahin and others added 30 commits February 8, 2021 10:19
@shcheklein shcheklein temporarily deployed to dvc-landing-ug-basic-co-ronvhm February 11, 2021 08:54 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-ug-basic-co-ronvhm February 11, 2021 09:38 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-ug-basic-co-ronvhm February 11, 2021 14:29 Inactive
@iesahin
Copy link
Contributor Author

iesahin commented Feb 11, 2021

I checked src/gatsby/models/glossary/index.js for failing codeclimate test and decided splitting the mentioned createSchemaCustomization function actually increases the complexity. Leaving it as it is.

@shcheklein shcheklein temporarily deployed to dvc-landing-ug-basic-co-ronvhm February 11, 2021 14:41 Inactive
@iesahin iesahin changed the title [WIP] Basic Concepts Documents for Status Check Basic Concepts Section for the Documentation Feb 11, 2021
@shcheklein shcheklein temporarily deployed to dvc-landing-ug-basic-co-ronvhm February 11, 2021 14:57 Inactive
@shcheklein
Copy link
Member

@iesahin could we take the JS part as a separate PR please and describe what it does? (Overall a bit of preference here - it's easier for us to review and merge smaller and focused PRs)

Comment on lines +95 to +113
"label": "Basic Concepts",
"slug": "concepts",
"source": false,
"children": [
"workspace",
"dvc-project",
"dependency",
"external-dependency",
"data-pipelines",
"stage",
"dvc-cache",
"dvc-metafiles",
"remote-storage",
"import-stage",
"metrics-and-plots",
"parameters",
"run-cache"
]
},
Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2021

Choose a reason for hiding this comment

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

Too many changes in a single PR 🙂 Let's start with the js engine changes as Ivan suggested (for this PR)? Then (separate PR) the 1-3 most important concept pages — that in itself will be a big contribution.

That said, I also have a couple comments on (1) the concept naming, and (2) level of abstraction:

  1. Let's use dvc- everywhere (where it can apply e.g. dvc-workspace?) or nowhere, I think.
  2. Not everything that has a tooltip needs a concept page e.g. "external dependency" is a bit specific and falls within the "dependency" concept. Not sure which ones to keep exactly, we can decide later.

Thanks

Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2021

Choose a reason for hiding this comment

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

Also on the level of abstraction: we could even consider putting related concepts together e.g. cache and remote, pipeline and stage (they could still each need separate tooltips though). WDYT? (also for later to worry about anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I agree with this. I tried to make minimal changes to file naming as I think that was a decided matter but we can change all to non-dvc versions IMO.
  2. You're also right on this but tooltips in the branch is using frontmatter and there can only be a single frontmatter for each page. We can either have a dependency tooltip/glossary and tell external dependency when the user visits the concept page, or all glosses should have separate pages. IMHO glossary/tooltips and concepts should be separated and we need to have 5-10 concept pages and put tooltips/glossary in a single file.
  3. This also needs separation of glossary and basic concepts. Current structure requires single page, short documents for concepts which has tooltips defined at the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

On 1. We're moving all the files to another directory I think so no problem in renaming them along the way, I think.
On 2. I think we can have empty concept md docs that only serve for tooltip purposes, but not link them in sidebar.js right?
On 3. True. For now we can pick which concept doc is more important and put other concepts in there (even if they have their own concept file that's only used for tooltips. Anyway, no need to worry about this for the first or 2nd PR probably...

Copy link
Contributor

Choose a reason for hiding this comment

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

p.s. the tooltip vs concept page separation part we can worry about when looking into #1395

Comment on lines +14 to +15
<!-- keywords: data pipeline, machine learning pipeline, devops for data science, devops for machine learning, "MLops", "what is a data pipeline?", "data pipeline examples", "machine learning (ML) workflow", "data science workflow", "data science pipeline workflow" -->

Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2021

Choose a reason for hiding this comment

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

So FYI (next PR) these are desired terms we want to try to get into the concept text for SEO purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Writing a document that puts all these words would be a challange 😄

Instead there can be a keywords: front matter that will be copied to meta tags of pages. Talking to search engine bots and talking to humans are different purposes IMO. These keywords can also be added to other pages.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2021

Choose a reason for hiding this comment

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

Search engines don't really care about metadata keywords anymore, we need to incorporate the keywords to the content (in a meaningful/truthful way) but yeah no need to have them all strictly, especially in small concept pages that will be impossible.

Comment on lines +16 to +20
# Data Pipelines

A data pipeline, in general, is a series of data processing <abbr>stages</abbr>
like data gathering, data transformation, model training, model testing and
reporting <abbr>metrics</abbr> and results. A pipeline may produce intermediate
Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 12, 2021

Choose a reason for hiding this comment

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

As you may have realized, a lot of the text in this branch comes from existing content, in this case https://dvc.org/doc/command-reference/dag.

So the (next) PR should also remove most of those explanations from the cmd ref. — another reason to focus on just one concept at a time per PR probably — not to mention all the link/tooltip updates that some of these concepts will require throughout 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'm not against using duplicate material as soon as it fits to the context. Concepts pages should be revised to contain less details but overall removing material from other parts of the documentation may not be necessary.

Users will be reading these sporadically and they probably will forget what they read in concepts when they are reading the command ref. Aesthetically it may not be pleasing to repeat ourselves but from POV of a user, these are separate instances of reading. Also we can't be sure the user will ever read the other if they read one of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

removing material from other parts of the documentation may not be necessary

That's actually an important goal for this task (see #550 (comment)). Some (a little) redundancy is OK but there should be a single source of full/comprehensive explanation for these concepts. Keep in mind that maintaining duplicated content is hard and error-prone 😅 (just like code)

Users will be reading these sporadically and they probably will forget

TBH I don't expect users to navigate to concepts much it at all (from nav). Like you suggested without context they're pretty hard to care about or understand. They'll probably only open them from links we place in other docs, and from the tooltips of course.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great though for this section to serve as an entry point. May be index page can give an overview of the most important ones? how do hey relate to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea. I agree about an guiding index page for those who do navigate in here. BUT probably the 1st PR (after this one) will only introduce 1 or a few concepts and the index may not make sense until we have most of them up.

@iesahin
Copy link
Contributor Author

iesahin commented Feb 12, 2021

I fully agree that this should be split into smaller PRs but this is a breaking change. JS part is actually copied from what @rogermparent did in ug-basic-concepts here

If we don't include JS changes to this, Basic Concepts don't appear in the sidebar and tooltips become large documents. JS part involves showing tooltip frontmatter instead of the whole page and actually directly copied from @rogermparent 's work.

Because of this I had to bundle all pages together. If we apply only the JS part and an .md file under /concepts/ dir doesn't have tooltip: frontmatter, it breaks the existing glossary/tooltip functionality. The content is also extended in most of the pages.

I'll be merging master into this branch and update the documents as you review but splitting this into smaller pieces will probably do more harm than good.

Thanks. @shcheklein @jorgeorpinel

@shcheklein
Copy link
Member

@iesahin

I fully agree that this should be split into smaller PRs but this is a breaking change. JS part is actually copied from what @rogermparent did in ug-basic-concepts here

makes sense! Can we make them as a separate PR still? May be just migrate existing tooltips to the new format? I'm happy to merge it with a content that just copies existing tooltips. Then we can proceed step by step improving it. WDYT?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 12, 2021

this is a breaking change
If we don't include JS changes to this, Basic Concepts don't appear in the sidebar and tooltips become large documents

@iesahin you can just leave all the json and js changes EXCEPT for sidebar.js AND move the existing md files (without changes) from basic-concepts/ to concepts/ for the first PR. That will preserve the existing tooltip functionality and enable editing concept pages and adding them to the nav in future PRs (I tried it locally and it seems to work). 👍

That's what I was referring to with "splitting the PR".

@jorgeorpinel
Copy link
Contributor

move the existing md files (without changes

No sorry that's not accurate. The only change would be to leave the new frontmatter (with tooltip fields) but the md content of all those pages would be empty initially.

@iesahin
Copy link
Contributor Author

iesahin commented Feb 13, 2021

It will be an interesting cherry-picking experience :)

I'll send you another PR by cherry picking the tooltip on the front matter functionality but without the concept pages in sidebar. That's the deal I think.

@iesahin
Copy link
Contributor Author

iesahin commented Feb 13, 2021

Also I'll try to fix prettier by changing its config because it's good test.

This is in another PR. #2194

@iesahin
Copy link
Contributor Author

iesahin commented Feb 13, 2021

I split JS parts, didn't touch current text in master but moved them to tooltip: front matter. Submitted #2192 for this. We can close this.

@iesahin iesahin closed this Feb 13, 2021
@jorgeorpinel
Copy link
Contributor

Thanks @iesahin !

@jorgeorpinel jorgeorpinel deleted the ug-basic-concepts-feb-21 branch July 29, 2022 17:07
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.

4 participants