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

Add params #1128

Merged
merged 13 commits into from
Apr 11, 2020
Merged

Add params #1128

merged 13 commits into from
Apr 11, 2020

Conversation

dmpetrov
Copy link
Member

@dmpetrov dmpetrov commented Apr 9, 2020

Params and params diff

@dmpetrov dmpetrov requested a review from jorgeorpinel April 9, 2020 07:35
@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 9, 2020 07:35 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 9, 2020 08:07 Inactive
@@ -284,6 +284,17 @@
"label": "move",
"slug": "move"
},
{
"label": "param",
Copy link
Member

Choose a reason for hiding this comment

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

params?

@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 01:15 Inactive
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 looks great to me

  • maybe some links from params diff to the params index description (which gives an excellent overview of what params are). May be just repeat some paragraphs.

One typo in sidebar.

  • Also, we need to update prism.js scripts to highight these new commands cc @jorgeorpinel

@shcheklein
Copy link
Member

shcheklein commented Apr 10, 2020

@dmpetrov thanks 🙏 .. really appreciate doing this.

@jorgeorpinel it should become part of the Experiments section of the new Get started tutorial. UPDATE: Added checkbox in #1074

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.

Phew... I have lots of questions and suggestions, sorry! 😅

Please just mostly notice the questions in bullets

But feel free to comment on any other comment. I can take over this PR and address all the other stutff. Much of it should be easy to apply since I left GH suggestions so feel fee to commit some of those as well.

content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/run.md Outdated Show resolved Hide resolved
content/docs/command-reference/run.md Outdated Show resolved Hide resolved
content/docs/command-reference/run.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 06:04 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 06:06 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 06:06 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 06:08 Inactive
Comment on lines 9 to 16
usage: dvc run [-h] [-q | -v] [-d DEPS] [-o OUTS] [-O OUTS_NO_CACHE]
[-p PARAMS] [-m METRICS] [-M METRICS_NO_CACHE] [-f FILE]
[-c CWD] [-w WDIR] [--no-exec] [-y] [--overwrite-dvcfile]
[--ignore-build-cache] [--remove-outs] [--no-commit]
[--outs-persist OUTS_PERSIST]
[--outs-persist-no-cache OUTS_PERSIST_NO_CACHE]
[--always-changed]
command
Copy link
Contributor

@jorgeorpinel jorgeorpinel Apr 10, 2020

Choose a reason for hiding this comment

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

One last comment: dvc run is getting gargantuan 🐋

Isn't there an issue out there somewhere to split it into 2 commands? May be time to revisit that 😬

@jorgeorpinel jorgeorpinel mentioned this pull request Apr 10, 2020
5 tasks
@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 08:33 Inactive
@dmpetrov

This comment has been minimized.

@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 23:36 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-params-3y8jqgjnupp April 10, 2020 23:44 Inactive
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 feedback for myself to address, please ignore.

Thanks for all the material @dmpetrov this is really complete!

  • We may also need to update the DVC-file format doc to include info about params field.

content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/params/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/run.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 11, 2020 06:57 Inactive
fix synopsis sections of list, update cmd refs
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-params-3y8jqgjnupp April 11, 2020 23:00 Inactive
@jorgeorpinel jorgeorpinel merged commit b77898f into master Apr 11, 2020
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Apr 11, 2020

Did lot's of editing in db3989d in case you want to check it @shcheklein @dmpetrov

Comment on lines +34 to +37
Supported parameter _value_ types are: string, integer, float, and arrays. DVC
itself does not ascribe any specific meaning for these values. They are
user-defined, and serve as a way to generalize and parametrize an machine
learning algorithms or data processing code.
Copy link
Contributor

Choose a reason for hiding this comment

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

The only question I still have is about this paragraph. What do we mean supported? Isn't any valid YAML or JSON value automatically supported? Supported for what, for params diff? If so maybe we should only mention this in that other doc (I did copy part of this p to there already). @dmpetrov

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