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

metrics: Use "metrics.json" as DEFAULT_METRICS_FILE. #8484

Closed
wants to merge 1 commit into from

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Oct 28, 2022

Closes #6168

@daavoo daavoo marked this pull request as draft October 28, 2022 10:59
@daavoo daavoo requested a review from dberenbaum October 28, 2022 10:59
Copy link
Contributor Author

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

I was thinking about how DEFAULT_PARAMS_FILE affects the dvc.yaml definition but I don't think it makes much sense to change metrics to also support a "granular" definition.

Feels overkill to implement:

metrics:
  - metrics.json:
      foo

And use the default file to make it equivalent to:

metrics:
  - foo

@daavoo daavoo marked this pull request as ready for review October 28, 2022 11:06
@daavoo daavoo force-pushed the default-metrics-file branch from 890fb65 to ef94e63 Compare October 28, 2022 11:06
@daavoo daavoo self-assigned this Oct 28, 2022
@daavoo daavoo added A: metrics Related to dvc metrics feature is a feature labels Oct 28, 2022
@pmrowla
Copy link
Contributor

pmrowla commented Oct 28, 2022

I know we just use .json for metrics everywhere already in our examples and tests, but I was wondering if it makes sense that we use different default formats for params and metrics. If we are actually going to define a default metrics file now, should we be using yaml for consistency across DVC instead of json?

@dberenbaum
Copy link
Collaborator

I was thinking about how DEFAULT_PARAMS_FILE affects the dvc.yaml definition but I don't think it makes much sense to change metrics to also support a "granular" definition.

Agreed that metrics don't need granular support.

should we be using yaml for consistency across DVC instead of json?

There's some related discussion in iterative/dvclive#292. I'd vote to keep as is since we already have it implemented that way in dvclive.


@daavoo Is this still desirable given the coming changes in #8371 and iterative/dvclive#311? My concern would be that we are adding defaults that do not match the dvclive outputs.

@daavoo
Copy link
Contributor Author

daavoo commented Oct 28, 2022

@daavoo Is this still desirable given the coming changes in #8371 and iterative/dvclive#311? My concern would be that we are adding defaults that do not match the dvclive outputs.

I was checking how hard was to do this and then combine it with #7939 in comparison to #8371

@dberenbaum
Copy link
Collaborator

@daavoo Is this still desirable given the coming changes in #8371 and iterative/dvclive#311? My concern would be that we are adding defaults that do not match the dvclive outputs.

I was checking how hard was to do this and then combine it with #7939 in comparison to #8371

It's more limited (for example, you can't have mutliple dvclive dirs), but I'm open to it as a first step if it's quicker. cc @skshetry

@daavoo
Copy link
Contributor Author

daavoo commented Nov 2, 2022

Closing in favor of top-level metrics

@daavoo daavoo closed this Nov 2, 2022
@daavoo daavoo deleted the default-metrics-file branch November 2, 2022 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: metrics Related to dvc metrics feature is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default metrics file
3 participants