Repository: iterative/dvc
Pull request: feat: ignore files not in remote when push is false
Fixes #10317 

Enables opt-in to remove `push: false` stage outputs from `not_in_remote` data status results.

Notable changes:
- Add `outs_no_push` to `dvc.stage.utils.fill_stage_outputs` keys, to facilitate making outputs with `push: false`.
- In `status`, when flag enabled, filter through files reported as `not_in_remote`, and remove them if not `can_push`.
- Add corresponding flag `--respect-no-push` flag to CLI

Open to suggestions on how to make the flag names more intuitive!

Corresponding PR for the docs: https://github.com/iterative/dvc.org/pull/5373


* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [x] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
data status returns files as "Not in remote" even though they are marked as push: false in pipeline
# Bug Report

<!--
## data status: incorrectly marks files as "Not in remote"
-->

## Description

`dvc data status --not-in-remote` is used to determine if some files haven't been pushed up to remote storage. In a CI/CD context it is a useful check before accepting a PR. If a DVC pipeline has a stage with outputs marked `push: false`, those files will appear as "Not in remote". This makes it harder to detect when files that should be pushed to remote have not been (especially in an automated manner).

### Reproduce

1. Create a dvc pipeline with a stage output marked `push: false`
2. `dvc repro`
3. `dvc push`
4. `dvc data status --not-in-remote`

### Expected

No files should be marked as "Not in remote" OR an option should be provided to suppress those files.

### Environment information

N/A

**Output of `dvc doctor`:**

```console
DVC version: 3.43.1 (pip)
-------------------------
Platform: Python 3.11.6 on Linux-5.15.0-92-generic-x86_64-with-glibc2.35
Subprojects:
        dvc_data = 3.9.0
        dvc_objects = 3.0.6
        dvc_render = 1.0.1
        dvc_task = 0.3.0
        scmrepo = 2.1.1
Supports:
        http (aiohttp = 3.8.5, aiohttp-retry = 2.8.3),
        https (aiohttp = 3.8.5, aiohttp-retry = 2.8.3),
        s3 (s3fs = 2023.9.0, boto3 = 1.28.17)
Config:
        Global: /home/mermerico/.config/dvc
        System: /etc/xdg/dvc
Cache types: symlink
Cache directory: ext4 on /dev/md0p1
Caches: local
Remotes: s3
Workspace directory: ext4 on /dev/md0p1
Repo: dvc, git
Repo.site_cache_dir: /var/tmp/dvc/repo/97a24c320f9c7207aea41ca9a4dc4061
```

Hints from issue discussion:
This affects us as well. Has there been any progress on this?
I've started drafting a PR for this. Will update when/if I have the time.

Update: #10749 is now ready for review.

Commit message: Refactor test to use convention

Patch to review:
diff --git a/dvc/repo/data.py b/dvc/repo/data.py
index bc727c88c7..6387587f7e 100644
--- a/dvc/repo/data.py
+++ b/dvc/repo/data.py
@@ -4,6 +4,7 @@
 from typing import TYPE_CHECKING, Any, TypedDict, Union
 
 from dvc.fs.callbacks import DEFAULT_CALLBACK
+from dvc.repo.worktree import worktree_view
 from dvc.ui import ui
 
 if TYPE_CHECKING:
@@ -51,11 +52,8 @@ def _diff(
     *,
     granular: bool = False,
     not_in_cache: bool = False,
-    not_in_remote: bool = False,
-    remote_refresh: bool = False,
     callback: "Callback" = DEFAULT_CALLBACK,
 ) -> dict[str, list[str]]:
-    from dvc_data.index import StorageError
     from dvc_data.index.diff import UNCHANGED, UNKNOWN, diff
 
     ret: dict[str, list[str]] = {}
@@ -97,19 +95,6 @@ def _add_change(typ, change):
             # NOTE: emulating previous behaviour
             _add_change("not_in_cache", change)
 
-        try:
-            if (
-                not_in_remote
-                and change.old
-                and change.old.hash_info
-                and not old.storage_map.remote_exists(
-                    change.old, refresh=remote_refresh
-                )
-            ):
-                _add_change("not_in_remote", change)
-        except StorageError:
-            pass
-
         _add_change(change.typ, change)
 
     return ret
@@ -217,15 +202,47 @@ def _transform_git_paths_to_dvc(repo: "Repo", files: Iterable[str]) -> list[str]
     return [repo.fs.relpath(file, start) for file in files]
 
 
-def status(repo: "Repo", untracked_files: str = "no", **kwargs: Any) -> Status:
+def _get_entries_not_in_remote(
+    repo: "Repo",
+    granular: bool = False,
+    remote_refresh: bool = False,
+) -> list[str]:
+    """Get entries that are not in remote storage."""
+    from dvc_data.index import StorageKeyError
+
+    # View into the index, with only pushable entries
+    view = worktree_view(repo.index, push=True).data["repo"]
+
+    missing_entries = []
+    for key, entry in view.iteritems(shallow=not granular):
+        if not (entry and entry.hash_info):
+            continue
+
+        k = (*key, "") if entry.meta and entry.meta.isdir else key
+        try:
+            if not view.storage_map.remote_exists(entry, refresh=remote_refresh):
+                missing_entries.append(os.path.sep.join(k))
+        except StorageKeyError:
+            pass
+
+    return missing_entries
+
+
+def status(
+    repo: "Repo",
+    untracked_files: str = "no",
+    not_in_remote: bool = False,
+    remote_refresh: bool = False,
+    granular: bool = False,
+    head: str = "HEAD",
+) -> Status:
     from dvc.scm import NoSCMError, SCMError
 
-    head = kwargs.pop("head", "HEAD")
-    uncommitted_diff = _diff_index_to_wtree(repo, **kwargs)
+    uncommitted_diff = _diff_index_to_wtree(repo, granular=granular)
     unchanged = set(uncommitted_diff.pop("unchanged", []))
 
     try:
-        committed_diff = _diff_head_to_index(repo, head=head, **kwargs)
+        committed_diff = _diff_head_to_index(repo, head=head, granular=granular)
     except (SCMError, NoSCMError):
         committed_diff = {}
     else:
@@ -234,10 +251,21 @@ def status(repo: "Repo", untracked_files: str = "no", **kwargs: Any) -> Status:
     git_info = _git_info(repo.scm, untracked_files=untracked_files)
     untracked = git_info.get("untracked", [])
     untracked = _transform_git_paths_to_dvc(repo, untracked)
+
+    entries_not_in_remote = (
+        _get_entries_not_in_remote(
+            repo,
+            granular=granular,
+            remote_refresh=remote_refresh,
+        )
+        if not_in_remote
+        else []
+    )
+
     # order matters here
     return Status(
         not_in_cache=uncommitted_diff.pop("not_in_cache", []),
-        not_in_remote=uncommitted_diff.pop("not_in_remote", []),
+        not_in_remote=entries_not_in_remote,
         committed=committed_diff,
         uncommitted=uncommitted_diff,
         untracked=untracked,
diff --git a/tests/func/test_data_status.py b/tests/func/test_data_status.py
index b7d0c97c41..1779ecd27d 100644
--- a/tests/func/test_data_status.py
+++ b/tests/func/test_data_status.py
@@ -1,13 +1,19 @@
 from os import fspath
 from os.path import join
+from typing import TYPE_CHECKING
 
 import pytest
 
 from dvc.repo import Repo
 from dvc.repo.data import _transform_git_paths_to_dvc, posixpath_to_os_path
-from dvc.testing.tmp_dir import make_subrepo
+from dvc.testing.tmp_dir import TmpDir, make_subrepo
 from dvc.utils.fs import remove
 
+if TYPE_CHECKING:
+    from pytest_test_utils import matchers
+
+    from dvc.stage import Stage
+
 EMPTY_STATUS = {
     "committed": {},
     "uncommitted": {},
@@ -423,6 +429,51 @@ def test_missing_remote_cache(M, tmp_dir, dvc, scm, local_remote):
     }
 
 
+def test_not_in_remote_respects_not_pushable(
+    M: type["matchers.Matcher"], tmp_dir: TmpDir, dvc: Repo, scm, mocker, local_remote
+):
+    stages: list[Stage] = tmp_dir.dvc_gen({"foo": "foo", "dir": {"foobar": "foobar"}})
+    # Make foo not pushable
+    stages[0].outs[0].can_push = False
+    stages[0].dump()
+
+    def assert_not_in_remote_is(
+        granular: bool, not_in_remote: list[str], committed: list[str]
+    ):
+        assert dvc.data_status(
+            granular=granular, remote_refresh=True, not_in_remote=True
+        ) == {
+            **EMPTY_STATUS,
+            "git": M.dict(),
+            "not_in_remote": M.unordered(*not_in_remote),
+            "committed": {"added": M.unordered(*committed)},
+        }
+
+    foo = "foo"
+    dir_ = join("dir", "")
+    foobar = join("dir", "foobar")
+
+    assert_not_in_remote_is(
+        granular=True,
+        not_in_remote=[dir_, foobar],
+        committed=[foo, dir_, foobar],
+    )
+    assert_not_in_remote_is(granular=False, not_in_remote=[dir_], committed=[foo, dir_])
+
+    dvc.push()
+
+    assert_not_in_remote_is(
+        granular=True,
+        not_in_remote=[],
+        committed=[foo, dir_, foobar],
+    )
+    assert_not_in_remote_is(
+        granular=False,
+        not_in_remote=[],
+        committed=[foo, dir_],
+    )
+
+
 def test_root_from_dir_to_file(M, tmp_dir, dvc, scm):
     tmp_dir.dvc_gen({"data": {"foo": "foo", "bar": "bar"}})
     remove("data")


Code review comments:
diff_hunk: @@ -234,10 +251,21 @@ def status(repo: "Repo", untracked_files: str = "no", **kwargs: Any) -> Status:
     git_info = _git_info(repo.scm, untracked_files=untracked_files)
     untracked = git_info.get("untracked", [])
     untracked = _transform_git_paths_to_dvc(repo, untracked)
+
+    entries_not_in_remote = (
+        _get_entries_not_in_remote(
+            repo,
+            granular=granular,
+            remote_refresh=remote_refresh,
+        )
+        if not_in_remote
+        else []
+    )
path: dvc/repo/data.py
text: @user1:
Can you please move this after `_diff_index_to_wtree` operation? I missed this.

When dvc switches to Git `HEAD` (or, switches back) as it does here in `_diff_head_to_index`, cached `repo.index` gets reset, which means we are re-reading `dvc.yaml`/`.dvc`/`dvc.lock` files again.