Skip to content

Misc. updates (2.0ish) #2062

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

Merged
merged 41 commits into from
Jan 5, 2021
Merged

Misc. updates (2.0ish) #2062

merged 41 commits into from
Jan 5, 2021

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Dec 28, 2020

@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-3ob4u99krie7 December 28, 2020 02:24 Inactive
@shcheklein shcheklein had a problem deploying to dvc-landing-jorge-4fnatpd3dlzo December 28, 2020 02:32 Failure
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-4fnatpd3dlzo December 28, 2020 02:36 Inactive
jorgeorpinel added a commit that referenced this pull request Dec 28, 2020
@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-eqcazokpzjdf December 28, 2020 03:07 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review December 28, 2020 03:09
@jorgeorpinel jorgeorpinel changed the title Misc. updates (2.0) Misc. updates (2.0ish) Dec 28, 2020
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 3, 2021 20:33 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 3, 2021 20:50 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 3, 2021 20:54 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 3, 2021 21:07 Inactive
arguments for `--targets` before `revisions`, you should use `--` after this
option's arguments, e.g.:
- `--targets <paths>` - specify the command's scope to these metrics files. It
accepts `paths` to any valid metrics file, regardless of whether it's used by
Copy link
Member

Choose a reason for hiding this comment

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

I think not used by DVC is not very clear, can we rephrase it somehow?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 4, 2021

Choose a reason for hiding this comment

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

OK changed to "regardless of whether dvc.yaml is currently tracking any metrics in them". WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

it's not even about dvc.yaml at this point, even DVC repo might not be initialized. I see that the whole change for that ticket might be out of scope since it's a much bigger change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right but yes, since iterative/dvc/issues/4446 is still open and the core team hasn't really sent many docs updates per their work on that... We could leave for later.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 5, 2021

Choose a reason for hiding this comment

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

Then again since I just added that note about not needing a DVC project... This text in --targets is kind of contradicting again... I'll simplify it a little for now (less specific).

UPDATE: Actually, I'm not sure how to generalize since we didn't want to say "not used by DVC". Sry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.s. the current text is not incorrect so... I think it should do for now 🙂

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 3, 2021 23:51 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 4, 2021 00:04 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 4, 2021 00:10 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 4, 2021 00:14 Inactive
jorgeorpinel added a commit to iterative/dvc that referenced this pull request Jan 4, 2021
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 4, 2021 00:33 Inactive

❗ By default it only shows parameters that were changed.
This includes everything in `params.yaml` (default parameters file) as well
everything listed in the `params` field of `dvc.yaml`/`dvc.lock`. Only params
Copy link
Member

Choose a reason for hiding this comment

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

still don't understand what does as well everything listed in the paramsfield ofdvc.yaml/dvc.lock`` mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

params diff withuot arguments shows everything changed in params.yaml as well as all params defined in dvc.yaml. And soon also even params used to templetize dvc.yaml (but that's not in scope, more for #2052).

I simplified to "...as well all the params used in dvc.yaml." and separated the mention of dvc.lock into a following sentence.

stages:
preprocess:
cmd: ./prepare.py
params:
Copy link
Member

Choose a reason for hiding this comment

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

we use epochs, learning-rate, batch_siz above, let's do the same in the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is way better 😅. Done!

DVC saves the param names and their latest values in the `dvc.yaml` file. These
values will be compared to the ones in the params files to determine if the
stage is invalidated upon pipeline [reproduction](/doc/command-reference/repro).
In contrast to a regular <abbr>dependency</abbr>, a parameter is not a file or
Copy link
Member

Choose a reason for hiding this comment

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

a parameter dependency ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

specific meaning to these values.

DVC saves parameter names and values in the project's
[DVC files](/doc/user-guide/dvc-files) in order to track them over time. They
Copy link
Member

Choose a reason for hiding this comment

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

in this case we can be specific? dvc.lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Specified.

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.

Some minor things left. I suggest to address the whole story related to non DVC repos for metrics/plots/etc separately. It's a bigger problem that we can handle for Misc updates.

efiop pushed a commit to iterative/dvc that referenced this pull request Jan 4, 2021
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 5, 2021 05:16 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 5, 2021 05:27 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-y9v9tpgve3zw January 5, 2021 06:01 Inactive
@jorgeorpinel
Copy link
Contributor Author

address the whole story related to non DVC repos for metrics/plots/etc separately

Agree. I'm hoping to do so as docs changes from core PRs related to iterative/dvc/issues/4446 continue to arrive.

Merging!

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.

2 participants