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

guide: document matrix in dvc.yaml #4761

Merged
merged 5 commits into from
Aug 17, 2023
Merged

guide: document matrix in dvc.yaml #4761

merged 5 commits into from
Aug 17, 2023

Conversation

skshetry
Copy link
Member

@skshetry skshetry requested a review from dberenbaum August 14, 2023 13:28
@shcheklein shcheklein temporarily deployed to dvc-org-fix-4741-jgwecqrfam22v August 14, 2023 13:29 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023

Link Check Report

There were no links to check!

Comment on lines 804 to 805
Note that templating is supported inside `matrix` and in definition, so you can
reference things defined in `vars` and from your imported file as usual.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by your imported file? Should we also mention that you can reference params files? It would be great to have a more complete example here (show the vars section and/or params.yaml file).

Copy link
Member Author

Choose a reason for hiding this comment

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

With imported, I meant vars loaded from files. But yes, it is confusing. I'll just get rid of it, and replace with an example of vars from params.yaml. Others should be left to the readers/users I think.

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Thanks @skshetry! Nice examples.

Let's at least mention in foreach that matrix now provides a more powerful way to define multiple stages in a single entry. I think we should go even further and say that foreach may be deprecated in future versions, so we recommend using matrix instead.

Co-authored-by: Dave Berenbaum <[email protected]>
@shcheklein shcheklein temporarily deployed to dvc-org-fix-4741-pcyzmkjun34oq August 15, 2023 15:12 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-fix-4741-pcyzmkjun34oq August 15, 2023 15:29 Inactive
@skshetry
Copy link
Member Author

Let's at least mention in foreach that matrix now provides a more powerful way to define multiple stages in a single entry. I think we should go even further and say that foreach may be deprecated in future versions, so we recommend using matrix instead.

I'll just recommend for now. Let's get some feedback first. :)

@shcheklein shcheklein temporarily deployed to dvc-org-fix-4741-pcyzmkjun34oq August 16, 2023 08:53 Inactive
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Definitely not a blocker for this P.R. but #3670 feels now even more important to address.
Too much important info in a place "too hidden"

Comment on lines 783 to 788
- _train@cnn-feature1_
- _train@cnn-feature2_
- _train@cnn-feature3_
- _train@xgb-feature1_
- _train@xgb-feature2_
- _train@xgb-feature3_
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change to a code block like this just to save space? The bullet points take up a lot space.

$ dvc stage list
train@cnn-feature1  Outputs cnn.pkl
train@cnn-feature2  Outputs cnn.pkl
train@cnn-feature3  Outputs cnn.pkl
train@xgb-feature1  Outputs xgb.pkl
train@xgb-feature2  Outputs xgb.pkl
train@xgb-feature3  Outputs xgb.pkl
Screenshot 2023-08-16 at 3 36 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed the change myself since I don't know how to add a code block in a suggestion, and I'm assuming this isn't a controversial change.

@shcheklein shcheklein temporarily deployed to dvc-org-fix-4741-pcyzmkjun34oq August 16, 2023 19:46 Inactive
Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Thanks @skshetry!

As a follow-up (unless you want to include in this PR), let's also add an example showing how it works with dict unpacking.

@skshetry skshetry merged commit 9e0c756 into main Aug 17, 2023
@skshetry skshetry deleted the fix-4741 branch August 17, 2023 05:10
@skshetry
Copy link
Member Author

skshetry commented Aug 17, 2023

Definitely not a blocker for this P.R. but #3670 feels now even more important to address.
Too much important info in a place "too hidden"

As a follow-up (unless you want to include in this PR), let's also add an example showing how it works with dict unpacking.

I think there are three parts here on how we should decouple:

  1. Expressions (whatever inside ${}), and dict unpacking.
  2. Templating, foreach and matrix
  3. Stage/Output schema

To me, it feels like matrix example belongs in dict unpacking rather than matrix (which is hidden right now).

@dberenbaum
Copy link
Contributor

@skshetry Why don't we discuss in #3670?

BTW, I don't understand what you mean by this:

matrix (which is hidden right now).

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.

dvc matrix
4 participants