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.

Sign up for GitHub

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

Jump to bottom

api: create docs #908

Merged
merged 109 commits into from
Mar 8, 2020
Merged

api: create docs #908

merged 109 commits into from
Mar 8, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jan 8, 2020 •
edited
Loading

Closes #463

See #463 (comment) for initial spec.

@shcheklein shcheklein temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 8, 2020 20:34 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 8, 2020 21:28 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 8, 2020 21:34 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 04:47 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 05:28 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 05:34 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 05:44 Inactive
@jorgeorpinel jorgeorpinel mentioned this pull request Jan 9, 2020
Closed
1 task
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 19:51 Inactive
jorgeorpinel
jorgeorpinel commented Jan 9, 2020
View reviewed changes
Copy link
Contributor Author

@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 Qs. Will try to answer myself but any answers are welcome.

public/static/docs/api-reference/get_url.md Outdated Show resolved Hide resolved
public/static/docs/api-reference/get_url.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel requested a review from Suor January 9, 2020 19:58
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 22:08 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-fd93qhq3ndrjud January 9, 2020 22:15 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review January 9, 2020 22:16
shcheklein
shcheklein reviewed Mar 5, 2020
View reviewed changes
public/static/docs/api-reference/open.md Outdated

- `dvc.exceptions.NoRemoteError` - no `remote` is found.

## Example: Use data tracked in a DVC repository online
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand the name, to be honest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "Use data from a DVC repository online"?

Copy link
Member

Choose a reason for hiding this comment

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

I would remove online ... instead, in rare cases we can figure out how to specify "offline"/local repos ... if it's needed at all

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 see the word "online" is throwing you off. I removed it from the text below but I'm not sure how best to replace it here. I can't say "remote DVC repo" because that's confusing: DVC remotes are not repositories but storage (cache backups, basically).
"...repo on the cloud" ? Sounds like marketing.
"...repo on Github" ? Too specific for the title.
Notice the word "online" is also used in the explanation of the repo param, so maybe it's OK to keep it here too.

Copy link
Member

Choose a reason for hiding this comment

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

Just omit it? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I guess using local repos is an edge case? Currently that's only shown as a side note in the 2nd example (and explained in the remote param). Removed.

shcheklein
shcheklein reviewed Mar 5, 2020
View reviewed changes
public/static/docs/api-reference/open.md
'get-started/data.xml',
repo='https://github.com/iterative/dataset-registry'
) as fd:
xmldom = parse(fd)
Copy link
Member

Choose a reason for hiding this comment

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

super relevant example would to show SAX or StAX parser instead of a DOM one - that's where it shines. Or we can make CSV example the main one and show how we process it in steam fashion (e.g. calculating sum or avg) - it would show the "streaming" aspect of the open() way better.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

make CSV example the main one and show how we process it in steam fashion (e.g. calculating sum or avg

Thinking about this, I don't think we're talking about real-time data streaming (e.g. from a Kafka server) so that continuously calculating a metric would be logical. Or maybe I missed the point?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I merged the PR, but still thinking about this one. What's the advantage of streaming files in open/read? Probably just making a big file available quickly so you can start processing it before it's all downloaded, but again, I don't think you'll want to show the progress of such processing, or is that a major use case you guys see? Cc @Suor @shcheklein

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this discussion to a new PR: #1037

shcheklein
shcheklein requested changes Mar 5, 2020
View reviewed changes
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

PTAL ... we are almost there - this time mostly some content improvements, minor fixes

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-ptgtvsucl2zs88 March 5, 2020 21:29 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-ptgtvsucl2zs88 March 8, 2020 01:11 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-ptgtvsucl2zs88 March 8, 2020 01:16 Inactive
shcheklein
shcheklein approved these changes Mar 8, 2020
View reviewed changes
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

It's good to merge. Up to you to address other comments that left.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-api-ptgtvsucl2zs88 March 8, 2020 01:45 Inactive
@jorgeorpinel jorgeorpinel dismissed Suor’s stale review March 8, 2020 22:56

Thanks for the review. Seems like it's all addressed and also per #908 (review) this is marked mergeable.

@jorgeorpinel jorgeorpinel merged commit 8bba8e8 into master Mar 8, 2020
jorgeorpinel added a commit that referenced this pull request Mar 8, 2020
@jorgeorpinel
api ref: change XML example in open to use SAX
4a8111e 
per #908 (comment)
@jorgeorpinel jorgeorpinel mentioned this pull request Mar 8, 2020
Merged
efiop added a commit to iterative/dvc that referenced this pull request Mar 11, 2020
@jorgeorpinel @efiop
api: docstring updates (2) (#3426)
9944efb 
* api: update to latest definitions to match iterative/dvc.org/pull/908

* Update dvc/api.py

* metrics show: update -h output to match docs
per iterative/dvc.org@2c34521

* api: update docstrings to match iterative/dvc.org/pull/908

* api: refactor DVC repo check in get_url, and document it

* dvc: cosmetic edits as I explored exceptions that api functions may raise

* api: copy default info to read() docstring from open()
per #3426 (review)

* api: improve open() docstring for clarity and add example
per #3426 (review)

* api: remove unnecessary info from get_url docstring
per #3426 (comment)

* api: produces->generated in open() docstring

* api: simplify open docstring
per #3426 (comment)

Co-authored-by: Ruslan Kuprieiev <[email protected]>
efiop added a commit to iterative/dvc that referenced this pull request Oct 12, 2020
@jorgeorpinel @efiop
term: "metrics" plural in output messages (#4699)
3b6a1c4 
* api: update to latest definitions to match iterative/dvc.org/pull/908

* Update dvc/api.py

* metrics show: update -h output to match docs
per iterative/dvc.org@2c34521

* api: update docstrings to match iterative/dvc.org/pull/908

* api: refactor DVC repo check in get_url, and document it

* dvc: cosmetic edits as I explored exceptions that api functions may raise

* api: copy default info to read() docstring from open()
per #3426 (review)

* api: improve open() docstring for clarity and add example
per #3426 (review)

* api: remove unnecessary info from get_url docstring
per #3426 (comment)

* api: produces->generated in open() docstring

* api: simplify open docstring
per #3426 (comment)

* term: "metrics" plural in output messages
iterative/dvc.org#1848 (review)

* typo

Co-authored-by: Ruslan Kuprieiev <[email protected]>
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) p1-important Active priorities to deal within next sprints C: ref Content of /doc/*-reference labels Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skshetry skshetry skshetry left review comments

@pared pared pared approved these changes

@efiop efiop efiop approved these changes

@shcheklein shcheklein shcheklein approved these changes

@Suor Suor Awaiting requested review from Suor

Assignees
No one assigned
Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference p1-important Active priorities to deal within next sprints
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

api: create documentation
7 participants
@jorgeorpinel @dmpetrov @shcheklein @Suor @efiop @pared @skshetry