feat: top level params and metrics #8529

skshetry · 2022-11-04T11:18:07Z

Opened for discussion. Currently, only top-level metrics/params are supported that are lists of strings.
dvc.yaml files are only discovered when they are at the root of the repo or if they contain at least one stage inside (this was the bug with top-level plots that dvcfiles are not discovered unless they have one stage).

TODOs

Collecting dvc.yaml files from other locations
Collecting top-level metrics/params/plots while collecting stages.
Finalizing schema/format
Fighting with path manipulation (it's seriously messed up ☢️).
Fighting with error collection handler
Templating?

skshetry · 2022-11-04T11:23:16Z

dvc/repo/params/show.py

+        rel_param_path = os.path.join(relpath, *repo.fs.path.parts(fs_path))
+        if not repo.fs.isfile(fs_path):
+            if repo.fs.isfile(rel_param_path):
+                fs_path = rel_param_path
+            else:
+                continue


I still don't understand how paths are supposed to work. :(

codecov · 2022-11-04T11:24:46Z

Codecov Report

Base: 94.13% // Head: 93.86% // Decreases project coverage by -0.27% ⚠️

Coverage data is based on head (8715d73) compared to base (aa2e830).
Patch coverage: 98.41% of modified lines in pull request are covered.

❗ Current head 8715d73 differs from pull request most recent head 3683530. Consider uploading reports for the commit 3683530 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8529      +/-   ##
==========================================
- Coverage   94.13%   93.86%   -0.28%     
==========================================
  Files         435      435              
  Lines       33335    33459     +124     
  Branches     4690     4711      +21     
==========================================
+ Hits        31379    31405      +26     
- Misses       1528     1609      +81     
- Partials      428      445      +17

Impacted Files	Coverage Δ
dvc/schema.py	`100.00% <ø> (ø)`
dvc/repo/stage.py	`91.44% <96.29%> (-1.11%)`	⬇️
dvc/repo/index.py	`95.60% <100.00%> (+0.46%)`	⬆️
dvc/repo/metrics/show.py	`95.34% <100.00%> (+0.47%)`	⬆️
dvc/repo/params/show.py	`91.91% <100.00%> (+0.71%)`	⬆️
dvc/stage/loader.py	`99.24% <100.00%> (+0.03%)`	⬆️
tests/func/metrics/test_diff.py	`100.00% <100.00%> (ø)`
tests/func/params/test_diff.py	`100.00% <100.00%> (ø)`
tests/unit/command/test_status.py	`100.00% <100.00%> (ø)`
tests/func/test_unprotect.py	`78.57% <0.00%> (-21.43%)`	⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dberenbaum · 2022-11-05T13:27:08Z

Looks good so far!

Templating?

Let's leave it out for the initial implementation.

Is the implementation connected to top-level plots? It would be good to have similar behavior between all the top-level keys.

skshetry · 2022-11-05T13:32:32Z

Is the implementation connected to top-level plots? It would be good to have similar behavior between all the top-level keys.

I duplicated the logic that’s in the plots. But we need to unify them which I’ll work next (which is the biggest part of this task).

Collecting dvc.yaml files from other locations

Collecting top-level metrics/params/plots while collecting stages

This should unify them, and also fix the plots bug.

dberenbaum · 2022-11-15T12:58:02Z

@skshetry What's the status here?

skshetry · 2022-11-16T09:22:22Z

Still working on the above two things, should have something by end of next week.

daavoo

@dberenbaum @skshetry

For the "Python Only" experiments, which I believe is currently the main driver for this, IMO the most critical TODOs are:

bug with top-level plots that dvcfiles are not discovered unless they have one stage.
Collecting dvc.yaml files from other locations

However, both can be worked around on the DVCLive side, so it would be great to get a minimal version of this P.R. merged as soon as possible to unblock.

II think it's ok if there are many pending follow-ups, as long as they don't break already existing user workflows.
We can not document this until follow-ups are done and only use it "hidden" in the DVCLive context in the meantime.

dberenbaum · 2022-11-25T15:07:22Z

Agreed @daavoo. Anything we can get merged would be helpful.

shcheklein · 2022-11-30T03:13:58Z

Collecting top-level metrics/params/plots while collecting stages

qq - does it imply moving the logic into the Index?

skshetry · 2022-11-30T05:51:40Z

Collecting top-level metrics/params/plots while collecting stages

qq - does it imply moving the logic into the Index?

yes, they will be moved to the Index, at least that's the plan.

skshetry · 2022-12-01T13:50:19Z

This should be ready for review. Although this works, this is a temporary solution. Overall I am not happy at all with the changes here.

I'll work on internal refactoring on index/loaders/dvcfile-api next.

skshetry · 2022-12-01T13:52:26Z

dvc/repo/stage.py

@@ -502,3 +502,104 @@ def is_out_or_ignored(root, directory):

    def collect_repo(self, onerror: Callable[[str, Exception], None] = None):
        return list(self._collect_repo(onerror))
+
+    def _load_file(self, path):


Duplicated from load_file above that also provides params/metrics/plots. Will refactor in successive PRs. This method serves _collect_all_from_repo below.

skshetry · 2022-12-01T13:53:07Z

dvc/repo/stage.py

+            stages.params_data,
+        )
+
+    def _collect_all_from_repo(


Bad naming I know, also duplicates logic from _collect_repo(). Again, will be refactored in successive PRs.

skshetry · 2022-12-01T13:53:57Z

dvc/stage/loader.py

+        self.metrics_data = self.data.get("metrics", [])
+        self.params_data = self.data.get("params", [])
+        self.plots_data = self.data.get("plots", {})


The whole PR is standing on these 3 lines. It feels weird to use StageLoader to get other sections, but this was a quick-and-easy solution.

Need to rearchitect here.

skshetry · 2022-12-01T13:55:35Z

dvc/repo/index.py

+    def _top_metrics(self):
+        self._collect()
+        return self._metrics
+
+    @cached_property
+    def _top_plots(self):
+        self._collect()
+        return self._plots
+
+    @cached_property
+    def _top_params(self):
+        self._collect()
+        return self._params
+
+    def _collect(self):
+        if "stages" in self.__dict__:
+            return self.stages


These all might be temporary, we probably should unify top-level metrics and stage-level metrics. Hence, keeping them as private.

skshetry · 2022-12-01T13:57:10Z

dvc/repo/index.py

+            metrics,
+            plots,
+            params,
+        ) = self.stage_collector._collect_all_from_repo(onerror=onerror)


Collection needs to be brought inside Index from Repo.stage. This seemed to require more change than I thought, as we have lots of dependencies on stage.collect(), which probably also needs to be changed/refactored.

dberenbaum · 2022-12-01T14:01:40Z

Thank you @skshetry!

Let's be clear that this is a prototype right now. We wanted to get something merged so we can start using it internally, but it's understood that it still needs a lot of polishing.

daavoo

I did some QA for the DVCLive use case and works as expected both on DVC UI and VSCode extension, including the handling of errors on collection.

Didn't look into the code deeply as per the comments of expected reactors soon.

skshetry commented Nov 4, 2022

View reviewed changes

skshetry force-pushed the toplevel branch from eb9c37b to 535bc9c Compare November 5, 2022 02:16

skshetry mentioned this pull request Nov 5, 2022

dvc.yaml: pipeline-less params and metrics #8371

Closed

daavoo force-pushed the toplevel branch from 535bc9c to 2f37b1b Compare November 25, 2022 11:26

daavoo reviewed Nov 25, 2022

View reviewed changes

daavoo mentioned this pull request Nov 30, 2022

Create DVC experiment on live.end. iterative/dvclive#366

Merged

skshetry force-pushed the toplevel branch from 2f37b1b to eb59c16 Compare December 1, 2022 10:02

skshetry changed the title ~~wip: top level params and metrics~~ feat: top level params and metrics Dec 1, 2022

skshetry requested review from dberenbaum, dtrifiro and daavoo December 1, 2022 13:50

skshetry commented Dec 1, 2022

View reviewed changes

skshetry added 4 commits December 1, 2022 15:11

wip: top level params and metrics

b294837

collect top-level params/plots/metrics together with stages

62d1203

add tests

b8400dc

fix windows path handling

3683530

daavoo force-pushed the toplevel branch from 0ce53c8 to 3683530 Compare December 1, 2022 14:11

dberenbaum approved these changes Dec 1, 2022

View reviewed changes

daavoo approved these changes Dec 1, 2022

View reviewed changes

daavoo enabled auto-merge (rebase) December 1, 2022 14:53

daavoo merged commit d57bb23 into iterative:main Dec 1, 2022

skshetry deleted the toplevel branch December 1, 2022 15:00

This was referenced Dec 2, 2022

plots: don't work unless the stages key is also in dvc.yaml. #8366

Closed

Top-level params and metrics iterative/dvcyaml-schema#26

Closed

This was referenced Dec 22, 2022

Top level metrics and params iterative/dvc.org#4180

Merged

params: clarify when they're granular iterative/dvc.org#4202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: top level params and metrics #8529

feat: top level params and metrics #8529

skshetry commented Nov 4, 2022 •

edited

Loading

skshetry Nov 4, 2022

codecov bot commented Nov 4, 2022 •

edited

Loading

dberenbaum commented Nov 5, 2022

skshetry commented Nov 5, 2022

dberenbaum commented Nov 15, 2022

skshetry commented Nov 16, 2022

daavoo left a comment

dberenbaum commented Nov 25, 2022

shcheklein commented Nov 30, 2022

skshetry commented Nov 30, 2022

skshetry commented Dec 1, 2022

skshetry Dec 1, 2022

skshetry Dec 1, 2022

skshetry Dec 1, 2022 •

edited

Loading

skshetry Dec 1, 2022

skshetry Dec 1, 2022

skshetry Dec 1, 2022

dberenbaum commented Dec 1, 2022

daavoo left a comment

feat: top level params and metrics #8529

feat: top level params and metrics #8529

Conversation

skshetry commented Nov 4, 2022 • edited Loading

TODOs

skshetry Nov 4, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 4, 2022 • edited Loading

Codecov Report

dberenbaum commented Nov 5, 2022

skshetry commented Nov 5, 2022

dberenbaum commented Nov 15, 2022

skshetry commented Nov 16, 2022

daavoo left a comment

Choose a reason for hiding this comment

dberenbaum commented Nov 25, 2022

shcheklein commented Nov 30, 2022

skshetry commented Nov 30, 2022

skshetry commented Dec 1, 2022

skshetry Dec 1, 2022

Choose a reason for hiding this comment

skshetry Dec 1, 2022

Choose a reason for hiding this comment

skshetry Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

skshetry Dec 1, 2022

Choose a reason for hiding this comment

skshetry Dec 1, 2022

Choose a reason for hiding this comment

skshetry Dec 1, 2022

Choose a reason for hiding this comment

dberenbaum commented Dec 1, 2022

daavoo left a comment

Choose a reason for hiding this comment

skshetry commented Nov 4, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading

skshetry Dec 1, 2022 •

edited

Loading