Repository: iterative/dvc

Issue:
dvc add absolute_path
`~/repo$ dvc add $HOME/repo/file` should realise that the file is not "external" and add it as a relative path rather than absolute.

Discussion:
casperdcl: also not sure whether this is a bug/feature request/enhancement :D
efiop: `dvc run` comes to mind as well. Same with `dvc import -o` and `dvc import-url`. You might want to just do that convertion in the OutputBASE/LOCAL class, when we mutating the `def_path`.


casperdcl: will that also affect paths written in `*.dvc` files?
efiop: @casperdcl Yes, but that is the point of this PR, right?
efiop: @casperdcl ?
casperdcl: > `dvc run` comes to mind as well. Same with `dvc import -o` and `dvc import-url`. You might want to just do that convertion in the OutputBASE/LOCAL class, when we mutating the `def_path`.

I think `dvc.output.local.OutputLOCAL` is already fine (there's a lot of complicated logic in it).

Just pushed an update for `dvc.output.base.OutputBASE`...
efiop: Thanks @casperdcl ! Looks good! Please see some comments above 🙂 
casperdcl: @efiop ok the style inconsistencies are now annoying. I feel like opening a new PR to fix things like relative imports and linting bash scripts...

Would you prefer I revert imports here?
efiop: @casperdcl We have issues for both of those already IIRC.

> Would you prefer I revert imports here?

Let them be this time, but please don't include things like that next time.

efiop: @Suor Using abs paths to in-repo files will cause adding them as external local outputs, which is not right. This change will break cases where there was a misuse, which is ok. There is no reason to drop this PR.
Suor: @efiop but is within and out of repo dir is not constant. 
efiop: @Suor Git doesn't have "external" stuff, but same as it, anything inside of our repo is an in-repo data, even if user has specified an absolute path. We've received reports from confused users, hence why we are fixing it here this way. 
casperdcl: I think maybe we could print a warning about converting abs => relative?

> Git doesn't have "external" stuff

Well actually I regularly do things like mess with `--local core.worktree` but in general I think it's safe to assume *"external" suff* isn't intended.
casperdcl: I think there are more people who e.g. drag-drop files into terminals (autofilling abs paths) and then expect them to be treated as relative to the repo than people who deliberately want in-repo paths to be tracked as external/absolute paths.
efiop: > I think maybe we could print a warning about converting abs => relative?

No need, when users add files this way they _expect_ them to become in-repo ones. Our current behavior is a bug, no need to put a warning on top. 🙂 

> Well actually I regularly do things like mess with --local core.worktree but in general I think it's safe to assume "external" suff isn't intended.

That is not the same "external" stuff that I'm talking about. `git add /absolute/out/of/repo/path` won't work, but `dvc add /absolute/out/of/repo/path` does.

So let's add a test here and we can merge it 🙂 

efiop: @casperdcl Also https://github.com/iterative/dvc/pull/2975#discussion_r368566499 
Suor: @casperdcl if you are going to add it to `OutputLocal` then it should be `OutputLocal._parse_path()`.
Suor: If we are going to fix this, we should not do that at `Output*` level, we should handle that somewhere closer to UI. `OutputBase/Local` doesn't know whether it got path from user or from `.dvc` file. 
efiop: @Suor There is `is_in_repo` already in Output*, handling abs path is suitable there as well.
casperdcl: Hmm. Maybe @efiop/@Suor you want to open an alternative PR and close this one? It looks like it's going to be just a couple of lines to change but requires a lot of discussion.
Suor: @efiop It could be easier to implement it in `Output*`, but it doesn't make it right. This will ignore whatever user wrote in `.dvc` file. Will rewrite it to rel path on `.dvc` file rewrite, but otherwise it will be silent.

So if this is user input bug then it should be handled where a user input is handled.
efiop: > It could be easier to implement it in Output*, but it doesn't make it right. This will ignore whatever user wrote in .dvc file. Will rewrite it to rel path on .dvc file rewrite, but otherwise it will be silent.

I think that it is alright. Can see someone writing a dvc file with `$(pwd)/output`, still expecting it to be handled properly as an in-repo file. So it is a user input bug and solving it in Output* will make it right once and for all.
efiop: @casperdcl Let's finish this one as proposed in this PR.
casperdcl: > Can see someone writing a dvc file with `$(pwd)/output`, still expecting it to be handled properly as an in-repo file

Not sure about that. If I manually edit a dvc-file by hand, I wouldn't want it to be silently changed.
casperdcl: @efiop the failing tests are the ones that need to be changed for the new behaviour. I'll fix them but first wanted some feedback on the implementation.
efiop: > Not sure about that. If I manually edit a dvc-file by hand, I wouldn't want it to be silently changed.

We do that in other fields, it is only changed when re-written.

> not sure about the distinction between def_path and path_info

`def_path` is what is recorded into the dvc-file, while `path_info` is a resolved path (e.g. we support stuff like `remote://mylocalremote/path`.
casperdcl: > We do that in other fields, it is only changed when re-written (e.g. on `dvc repro`).

Right sounds like discussion for a different issue.

> `def_path` is what is recorded into the dvc-file, while `path_info` is a resolved path (e.g. we support stuff like `remote://mylocalremote/path`.

So I think that means `path_info` should not be modified? i.e.

```python
# path_info before == after
assert self._parse_path(self.remote, self.def_path) == \
  self._parse_path(self.remote, relpath(self.def_path, self.repo.root_dir))
```
efiop: > So I think that means path_info should not be modified? i.e.

I'm pretty sure it doesn't matter, as long as def_path is the same, as path_infos will be used by abspath anyway.
Suor: Stumbled upon this while doing some updates to `dvcx` publishing, looks like we need to make `.is_in_repo` say `True` for absolute paths when that path is in repo root. This will trigger automatic in repo handling for `Stage.create(..., add=True)`, which will do the job naturally.
casperdcl: > Stumbled upon this while doing some updates to `dvcx` publishing, looks like we need to make `.is_in_repo` say `True` for absolute paths when that path is in repo root. This will trigger automatic in repo handling for `Stage.create(..., add=True)`, which will do the job naturally.

If that fixes this issue I'd say it's a bug because of

> def_path is resolved relative to `stage.wdir` when it is loaded, so it has to be relative to `stage.wdir` not the repo root

See https://github.com/iterative/dvc/pull/2975#discussion_r375414004
Suor: > If that fixes this issue I'd say it's a bug because of

If `def_path` or more precisely `dvc add` argument is absolute it is not resolved as relative to anything. So I can't see any bug.
efiop: > Stumbled upon this while doing some updates to dvcx publishing, looks like we need to make .is_in_repo say True for absolute paths when that path is in repo root. This will trigger automatic in repo handling for Stage.create(..., add=True), which will do the job naturally.

Good point, that might work, but I'm worried about `def_path` being not quire right until we actually write the dvc-file. Might be still suitable though.
casperdcl: I decided not to touch `is_in_repo` for now; not sure if changing its behaviour will break anything else.
efiop: @casperdcl `is_in_repo` now works automatically because you've adjusted def_path. :+1:

@Suor Adjusting `is_in_repo` will affect def_path only in `dumpd`, so effectively only after reloading, which is not the best way of handling it, as it will break the symmetry between "before" and "after" reloading. 

Patch:
diff --git a/dvc/analytics.py b/dvc/analytics.py
index 7c0a69310..76dbc335c 100644
--- a/dvc/analytics.py
+++ b/dvc/analytics.py
@@ -76,7 +76,10 @@ def send(report):
     headers = {"content-type": "application/json"}
 
     with open(report, "rb") as fobj:
-        requests.post(url, data=fobj, headers=headers, timeout=5)
+        try:
+            requests.post(url, data=fobj, headers=headers, timeout=5)
+        except requests.exceptions.RequestException:
+            logger.debug("failed to send analytics report", exc_info=True)
 
     os.remove(report)
 
diff --git a/dvc/logger.py b/dvc/logger.py
index 9385a6aa2..d1cf08e83 100644
--- a/dvc/logger.py
+++ b/dvc/logger.py
@@ -51,7 +51,6 @@ class ColorFormatter(logging.Formatter):
 
     color_code = {
         "DEBUG": colorama.Fore.BLUE,
-        "INFO": "",
         "WARNING": colorama.Fore.YELLOW,
         "ERROR": colorama.Fore.RED,
         "CRITICAL": colorama.Fore.RED,
@@ -59,29 +58,21 @@ class ColorFormatter(logging.Formatter):
 
     def format(self, record):
         msg = record.msg.format(*record.args) if record.args else record.msg
+        exception, stack_trace = self._parse_exc(record)
+        return ("{prefix}{description}{stack_trace}").format(
+            prefix=self._prefix(record),
+            description=self._description(msg, exception),
+            stack_trace=stack_trace,
+        )
 
+    def _prefix(self, record):
         if record.levelname == "INFO":
-            return msg
-
-        if record.levelname == "ERROR" or record.levelname == "CRITICAL":
-            exception, stack_trace = self._parse_exc(record)
-
-            return (
-                "{color}{levelname}{nc}: {description}" "{stack_trace}\n"
-            ).format(
-                color=self.color_code.get(record.levelname, ""),
-                nc=colorama.Fore.RESET,
-                levelname=record.levelname,
-                description=self._description(msg, exception),
-                msg=msg,
-                stack_trace=stack_trace,
-            )
+            return ""
 
-        return "{color}{levelname}{nc}: {msg}".format(
+        return "{color}{levelname}{nc}: ".format(
             color=self.color_code.get(record.levelname, ""),
-            nc=colorama.Fore.RESET,
             levelname=record.levelname,
-            msg=msg,
+            nc=colorama.Fore.RESET,
         )
 
     def _current_level(self):
diff --git a/dvc/output/local.py b/dvc/output/local.py
index 0e1b72dc9..6e4123d1e 100644
--- a/dvc/output/local.py
+++ b/dvc/output/local.py
@@ -18,6 +18,12 @@ class OutputLOCAL(OutputBase):
     REMOTE = RemoteLOCAL
     sep = os.sep
 
+    def __init__(self, stage, path, *args, **kwargs):
+        if stage and path_isin(path, stage.repo.root_dir):
+            path = relpath(path, stage.wdir)
+
+        super().__init__(stage, path, *args, **kwargs)
+
     def _parse_path(self, remote, path):
         parsed = urlparse(path)
         if parsed.scheme == "remote":
diff --git a/dvc/remote/gdrive.py b/dvc/remote/gdrive.py
index 9cbca729e..4bffb60b4 100644
--- a/dvc/remote/gdrive.py
+++ b/dvc/remote/gdrive.py
@@ -275,6 +275,13 @@ class RemoteGDrive(RemoteBASE):
         item.Upload()
         return item
 
+    @gdrive_retry
+    def delete_remote_file(self, remote_id):
+        param = {"id": remote_id}
+        # it does not create a file on the remote
+        item = self.drive.CreateFile(param)
+        item.Delete()
+
     @gdrive_retry
     def get_remote_item(self, name, parents_ids):
         if not parents_ids:
@@ -387,3 +394,7 @@ class RemoteGDrive(RemoteBASE):
             except ValueError:
                 # We ignore all the non-cache looking files
                 logger.debug('Ignoring path as "non-cache looking"')
+
+    def remove(self, path_info):
+        remote_id = self.get_remote_id(path_info)
+        self.delete_remote_file(remote_id)
diff --git a/dvc/version.py b/dvc/version.py
index 586e45286..089533383 100644
--- a/dvc/version.py
+++ b/dvc/version.py
@@ -7,7 +7,7 @@ import os
 import subprocess
 
 
-_BASE_VERSION = "0.82.9"
+_BASE_VERSION = "0.83.0"
 
 
 def _generate_version(base_version):
diff --git a/setup.py b/setup.py
index 1eac416a2..40b3b16c5 100644
--- a/setup.py
+++ b/setup.py
@@ -85,7 +85,7 @@ install_requires = [
 # Extra dependencies for remote integrations
 
 gs = ["google-cloud-storage==1.19.0"]
-gdrive = ["pydrive2>=1.4.2"]
+gdrive = ["pydrive2>=1.4.5"]
 s3 = ["boto3>=1.9.201"]
 azure = ["azure-storage-blob==2.1.0"]
 oss = ["oss2==2.6.1"]


Test patch:
diff --git a/tests/func/test_add.py b/tests/func/test_add.py
index 1ce44a0b0..002722f52 100644
--- a/tests/func/test_add.py
+++ b/tests/func/test_add.py
@@ -216,7 +216,7 @@ class TestAddLocalRemoteFile(TestDvc):
         self.assertEqual(ret, 0)
 
         d = load_stage_file("bar.dvc")
-        self.assertEqual(d["outs"][0]["path"], bar)
+        self.assertEqual(d["outs"][0]["path"], self.BAR)
 
 
 class TestCmdAdd(TestDvc):
diff --git a/tests/unit/output/test_local.py b/tests/unit/output/test_local.py
index 50059e6ea..ef029d294 100644
--- a/tests/unit/output/test_local.py
+++ b/tests/unit/output/test_local.py
@@ -42,13 +42,28 @@ def test_str_workdir_inside_repo(dvc):
     assert os.path.join("some_folder", "path") == str(output)
 
 
-def test_str_on_absolute_path(dvc):
+def test_str_on_local_absolute_path(dvc):
     stage = Stage(dvc)
 
-    path = os.path.abspath(os.path.join("path", "to", "file"))
-    output = OutputLOCAL(stage, path, cache=False)
+    rel_path = os.path.join("path", "to", "file")
+    abs_path = os.path.abspath(rel_path)
+    output = OutputLOCAL(stage, abs_path, cache=False)
 
-    assert path == str(output)
+    assert output.def_path == rel_path
+    assert output.path_info.fspath == abs_path
+    assert str(output) == rel_path
+
+
+def test_str_on_external_absolute_path(dvc):
+    stage = Stage(dvc)
+
+    rel_path = os.path.join("..", "path", "to", "file")
+    abs_path = os.path.abspath(rel_path)
+    output = OutputLOCAL(stage, abs_path, cache=False)
+
+    assert output.def_path == abs_path
+    assert output.path_info.fspath == abs_path
+    assert str(output) == abs_path
 
 
 class TestGetFilesNumber(TestDvc):
diff --git a/tests/unit/test_analytics.py b/tests/unit/test_analytics.py
index 3179aeff0..9413e781b 100644
--- a/tests/unit/test_analytics.py
+++ b/tests/unit/test_analytics.py
@@ -61,16 +61,19 @@ def test_runtime_info(tmp_global_config):
 
 @mock.patch("requests.post")
 def test_send(mock_post, tmp_path):
+    import requests
+
     url = "https://analytics.dvc.org"
     report = {"name": "dummy report"}
-    fname = str(tmp_path / "report")
+    report_file = tmp_path / "report"
 
-    with open(fname, "w") as fobj:
-        json.dump(report, fobj)
+    report_file.write_text(json.dumps(report))
+    mock_post.side_effect = requests.exceptions.RequestException
 
-    analytics.send(fname)
+    analytics.send(str(report_file))
     assert mock_post.called
     assert mock_post.call_args.args[0] == url
+    assert not report_file.exists()
 
 
 @pytest.mark.parametrize(
diff --git a/tests/unit/test_logger.py b/tests/unit/test_logger.py
index 0847a5e45..1b7d906e1 100644
--- a/tests/unit/test_logger.py
+++ b/tests/unit/test_logger.py
@@ -44,7 +44,7 @@ class TestColorFormatter:
         with caplog.at_level(logging.INFO, logger="dvc"):
             logger.error("message")
 
-            expected = "{red}ERROR{nc}: message\n".format(**colors)
+            expected = "{red}ERROR{nc}: message".format(**colors)
 
             assert expected == formatter.format(caplog.records[0])
 
@@ -55,7 +55,7 @@ class TestColorFormatter:
             except Exception:
                 logger.exception("message")
 
-            expected = "{red}ERROR{nc}: message\n".format(**colors)
+            expected = "{red}ERROR{nc}: message".format(**colors)
 
             assert expected == formatter.format(caplog.records[0])
 
@@ -66,7 +66,7 @@ class TestColorFormatter:
             except Exception:
                 logger.exception("")
 
-            expected = "{red}ERROR{nc}: description\n".format(**colors)
+            expected = "{red}ERROR{nc}: description".format(**colors)
 
             assert expected == formatter.format(caplog.records[0])
 
@@ -77,9 +77,7 @@ class TestColorFormatter:
             except Exception:
                 logger.exception("message")
 
-            expected = "{red}ERROR{nc}: message - description\n".format(
-                **colors
-            )
+            expected = "{red}ERROR{nc}: message - description".format(**colors)
 
             assert expected == formatter.format(caplog.records[0])
 
@@ -95,7 +93,7 @@ class TestColorFormatter:
                 "{red}ERROR{nc}: description\n"
                 "{red}{line}{nc}\n"
                 "{stack_trace}"
-                "{red}{line}{nc}\n".format(
+                "{red}{line}{nc}".format(
                     line="-" * 60, stack_trace=stack_trace, **colors
                 )
             )
@@ -114,7 +112,7 @@ class TestColorFormatter:
                 "{red}ERROR{nc}: something\n"
                 "{red}{line}{nc}\n"
                 "{stack_trace}"
-                "{red}{line}{nc}\n".format(
+                "{red}{line}{nc}".format(
                     line="-" * 60, stack_trace=stack_trace, **colors
                 )
             )
@@ -136,7 +134,7 @@ class TestColorFormatter:
                 "{red}ERROR{nc}: message - second: first\n"
                 "{red}{line}{nc}\n"
                 "{stack_trace}"
-                "{red}{line}{nc}\n".format(
+                "{red}{line}{nc}".format(
                     line="-" * 60, stack_trace=stack_trace, **colors
                 )
             )