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

Fix issue while sending analytics report #6407

Merged
merged 2 commits into from Aug 10, 2021

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Aug 10, 2021

When sending analytics, we are not flushing the data to the temp file
which causes failure at times, especially on Windows.

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

When sending analytics, we are not flushing the data to the temp file
which causes failure at times, especially on Windows
@skshetry skshetry requested a review from a team as a code owner August 10, 2021 09:18
@skshetry skshetry requested a review from pmrowla August 10, 2021 09:18
@skshetry
Copy link
Member Author

skshetry commented Aug 10, 2021

The test in de21ae4 demonstrates the issue with analytics. See https://github.com/iterative/dvc/actions/runs/1116040914.
Fixed by 9acf0d7.

Here's a snippet of the traceback:

------------------------------ Captured log call -------------------------------
ERROR    dvc:main.py:86 unexpected error
Traceback (most recent call last):
  File "/Users/runner/work/dvc/dvc/dvc/main.py", line 55, in main
    ret = cmd.do_run()
  File "/Users/runner/work/dvc/dvc/dvc/command/base.py", line 64, in do_run
    return self.run()
  File "/Users/runner/work/dvc/dvc/dvc/command/daemon.py", line 32, in run
    analytics.send(self.args.target)
  File "/Users/runner/work/dvc/dvc/dvc/analytics.py", line 67, in send
    report = json.load(fobj)
  File "/Users/runner/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/json/__init__.py", line 296, in load
    parse_constant=parse_constant, object_pairs_hook=object_pairs_hook, **kw)
  File "/Users/runner/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/Users/runner/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/Users/runner/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

This is happening because the file where we are saving the analytics report has been newly created (through NamedTemporaryFile) and we have written some contents to it, but the buffer is not yet flushed.

dvc/dvc/analytics.py

Lines 33 to 35 in 38cd7d0

with tempfile.NamedTemporaryFile(delete=False, mode="w") as fobj:
json.dump(report, fobj)
daemon(["analytics", fobj.name])

This way, the contents bufferred will get a chance to be flushed.
Comment on lines +30 to +35
def mock_daemon(mocker):
def func(argv):
return main(["daemon", *argv])

m = mocker.patch("dvc.daemon.daemon", mocker.MagicMock(side_effect=func))
yield m
Copy link
Member Author

Choose a reason for hiding this comment

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

Having a conditional daemonizer would be good for testing and QA, rather than mocking it. But didn't want to introduce big change.

@skshetry skshetry changed the title [WIP] Fix issue while sending analytics report Fix issue while sending analytics report Aug 10, 2021
@@ -32,7 +32,7 @@ def collect_and_send_report(args=None, return_code=None):

with tempfile.NamedTemporaryFile(delete=False, mode="w") as fobj:
json.dump(report, fobj)
daemon(["analytics", fobj.name])
daemon(["analytics", fobj.name])
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@skshetry skshetry self-assigned this Aug 10, 2021
@skshetry skshetry added bugfix fixes bug p0-critical Critical issue. Needs to be fixed ASAP. P: windows Related to the Platform: Windows labels Aug 10, 2021
@skshetry skshetry added this to In progress in DVC 10 Aug - 24 Aug 2021 via automation Aug 10, 2021
@skshetry skshetry removed this from In progress in DVC 10 Aug - 24 Aug 2021 Aug 10, 2021
@skshetry skshetry added this to In progress in DVC 27 Jul - 10 Aug via automation Aug 10, 2021
@skshetry skshetry moved this from In progress to Review in progress in DVC 27 Jul - 10 Aug Aug 10, 2021
@skshetry skshetry added this to In progress in DVC 10 Aug - 24 Aug 2021 via automation Aug 10, 2021
@skshetry skshetry moved this from Review in progress to Done in DVC 27 Jul - 10 Aug Aug 10, 2021
@efiop efiop merged commit fb96b13 into iterative:master Aug 10, 2021
DVC 10 Aug - 24 Aug 2021 automation moved this from In progress to Done Aug 10, 2021
@skshetry skshetry removed this from Done in DVC 10 Aug - 24 Aug 2021 Aug 10, 2021
@skshetry skshetry mentioned this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes bug P: windows Related to the Platform: Windows p0-critical Critical issue. Needs to be fixed ASAP.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants