Skip to content

add example with python parameters file#1799

Merged
shcheklein merged 1 commit into
treeverse:masterfrom
aandrusenko:py-params-support
Oct 6, 2020
Merged

add example with python parameters file#1799
shcheklein merged 1 commit into
treeverse:masterfrom
aandrusenko:py-params-support

Conversation

@aandrusenko
Copy link
Copy Markdown
Contributor

Related to treeverse/dvc#4456

Copy link
Copy Markdown
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.

Thank you @aandrusenko !

Missing a couple files:

  • \content\docs\start\experiments.md
  • \content\docs\user-guide\basic-concepts\parameter.md

Would you mind including them? I can do it if you're unable for any reason, no problem at all.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Sep 23, 2020

@shcheklein this makes me realize we only have params.yaml examples. Should we create a small Parameters files section with CSV, TOML, and Python examples in the Metafiles guide?

@shcheklein
Copy link
Copy Markdown
Contributor

shcheklein commented Sep 23, 2020

@jorgeorpinel I would not make it a priority, I hope users can figure the idea out from the YAML example.

  • Python is definitely a different beast and it's good to see how does it actually look like.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

OK. @aandrusenko would you like to try adding a Python params file example in content\docs\command-reference\params\index.md ? Again, I can do it if not. Thanks

@aandrusenko
Copy link
Copy Markdown
Contributor Author

aandrusenko commented Sep 23, 2020

@jorgeorpinel

  • \content\docs\start\experiments.md

was done

  • \content\docs\user-guide\basic-concepts\parameter.md

added

@aandrusenko
Copy link
Copy Markdown
Contributor Author

OK. @aandrusenko would you like to try adding a Python params file example in content\docs\command-reference\params\index.md ? Again, I can do it if not. Thanks

I have already added an example

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Oh, I see. I didn't notice it the first time I looked. OK, checking again now ⌛

Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment thread content/docs/command-reference/params/index.md Outdated
Comment on lines +193 to +210
Alternatively, the entire `TestConfig` group can be referenced, instead of the
parameters in it:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What other data structures are supported as groups other than classes @aandrusenko @efiop ? Thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only classes and dicts

Copy link
Copy Markdown
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.

Looks good. There's a couple minor questions left above which may need addressing but could be merged as-is too, probably.

Do you want to double check @shcheklein ? Thanks

@shcheklein
Copy link
Copy Markdown
Contributor

shcheklein commented Sep 29, 2020

I see that some checks failed? Also, please let's merge upstream master to run (new) link checks?

Comment thread content/docs/start/experiments.md Outdated
```

The following [stage](/doc/command-reference/run) depends on params
`IS_BOOL`, `CONST`, as well as `TrainConfig`'s `EPOCHS` and `layers`:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aandrusenko qq - if we depend on the instance variable (self.layers), when do we extract value? What is the semantics here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

From a class, you can extract either class constants or self variables defined in __init__

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, thanks! But what about the semantics? What if I have

self.layers = 10
self.layers = 12

or if I have

self.layers = 3+2

what are the expectations and limitations here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the first case, TrainConfig.layers will be 12. In the second, TrainConfig.layers will not be found because the complex expression

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's make it part of the docs and/or example? @aandrusenko it's totally fine if you don't have enough capacity, let us know and we'll take it over, but I think we should specify some basic semantics for this. Modifying example (add a few lines with Python comments + some text ?) should be enough, I guess.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shcheklein, I added more comments

Copy link
Copy Markdown
Contributor

@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.

we still have some open questions we need to answer (and potentially add details to the docs)

@efiop @aandrusenko it would be great if you could help us here

@aandrusenko
Copy link
Copy Markdown
Contributor Author

@shcheklein @jorgeorpinel I squashed changes and rebased

@shcheklein
Copy link
Copy Markdown
Contributor

Thanks @aandrusenko ! Looks like there is a conflict. We can fix it on my end, but feel free to rebase if you want.

@shcheklein
Copy link
Copy Markdown
Contributor

@aandrusenko perfect, thanks! 🙏

one last thing - something is happening with GH not attributing commits to you- could you please check your local and GH settings. Both should use the same email I think.

@aandrusenko
Copy link
Copy Markdown
Contributor Author

one last thing - something is happening with GH not attributing commits to you- could you please check your local and GH settings. Both should use the same email I think.

Oh, sorry, fixed it

@shcheklein shcheklein merged commit 07ebb65 into treeverse:master Oct 6, 2020
@shcheklein
Copy link
Copy Markdown
Contributor

thanks @aandrusenko !

Comment thread content/docs/command-reference/params/index.md
Copy link
Copy Markdown
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.

Post-merge review that I'll address myself. Includes some questions though:

Comment thread content/docs/command-reference/params/index.md
Comment thread content/docs/command-reference/params/index.md
Comment thread content/docs/command-reference/params/index.md
Comment on lines +172 to +179
def __init__(self):
# TrainConfig.layers param will be 9
self.layers = 5
self.layers = 9
# TrainConfig.foo will NOT be found because the complex expression
self.foo = 1 + 2
# TrainConfig.bar will NOT be found
bar = 1

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why can't DVC find vars that need evaluation though? Isn't the whole params file interpreted by Python first @aandrusenko @efiop? (Just curious) Thanks

Copy link
Copy Markdown
Contributor Author

@aandrusenko aandrusenko Oct 14, 2020

Choose a reason for hiding this comment

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

Nope, parameters are parsed using the ast module, it parses complex expressions into a complex structure that I did not support

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, thanks.

Comment thread content/docs/command-reference/params/index.md
jorgeorpinel added a commit that referenced this pull request Oct 17, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Oct 17, 2020
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.

3 participants