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 example with python parameters file #1799

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

aandrusenko
Copy link
Contributor

Related to iterative/dvc#4456

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.

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
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
Member

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
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
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
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
Contributor

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

Comment on lines 193 to 210
Alternatively, the entire `TestConfig` group can be referenced, instead of the
parameters in it:
Copy link
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
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
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
Member

shcheklein commented Sep 29, 2020

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

```

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

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
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
Member

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
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
Member

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

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
Contributor Author

@shcheklein @jorgeorpinel I squashed changes and rebased

@shcheklein
Copy link
Member

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
Member

@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
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 iterative:master Oct 6, 2020
@shcheklein
Copy link
Member

thanks @aandrusenko !

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.

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

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
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
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
Contributor

Choose a reason for hiding this comment

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

I see, thanks.

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