nightly robustness fixes for linking across devices #43771

scopatz · 2020-08-28T15:27:01Z

codecov · 2020-08-28T18:56:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6d31213). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #43771   +/-   ##
=========================================
  Coverage          ?   69.34%           
=========================================
  Files             ?      378           
  Lines             ?    46698           
  Branches          ?        0           
=========================================
  Hits              ?    32381           
  Misses            ?    14317           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d31213...677b919. Read the comment docs.

ezyang · 2020-08-28T20:32:27Z

tools/nightly.py

+    if platform.startswith("win"):
+        return False
+    src = os.path.join(source_dir, "__nightly_test__")
+    trg = os.path.join(target_dir, "__nightky_test__")


ezyang · 2020-08-28T20:34:07Z

Can you briefly describe how you tested this fixed the bug in question?

Also, another common fix for this cross-link problem is to just put the tmp files in the same directory as where the eventual location will be; see also https://rachelbythebay.com/w/2020/08/11/files/

scopatz · 2020-08-28T20:41:15Z

Can you briefly describe how you tested this fixed the bug in question?

I tested it locally as did @lezcano (who originally reported the issue).

Also, another common fix for this cross-link problem is to just put the tmp files in the same directory as where the eventual location will be; see also https://rachelbythebay.com/w/2020/08/11/files/

It is true, but I though this was a more reasonable solution given the existing architecture and that most of the files in the conda package will not actually be linked, so in the event of a failure they will be cleaned up by the OS automatically, whereas if the tmpdir is in the target dir then you are relying on Python to clean them up. Basically, this seems more robust overall and less likely to get the entirety of the nightly pytorch accidentally committed to the repo.

malfet · 2020-08-28T21:16:46Z

tools/nightly.py

+    src = os.path.join(source_dir, "__nightly_test__")
+    trg = os.path.join(target_dir, "__nightky_test__")
+    try:
+        with open(src, "w"):
+            pass
+        os.link(src, trg)
+        linkable = True
+    except OSError:
+        linkable = False
+    finally:
+        if os.path.isfile(trg):
+            os.remove(trg)
+        if os.path.isfile(src):
+            os.remove(src)
+    return linkable


Hmm, wouldn't using os.stat be a much simpler solution?

Suggested change

src = os.path.join(source_dir, "__nightly_test__")

trg = os.path.join(target_dir, "__nightky_test__")

try:

with open(src, "w"):

pass

os.link(src, trg)

linkable = True

except OSError:

linkable = False

finally:

if os.path.isfile(trg):

os.remove(trg)

if os.path.isfile(src):

os.remove(src)

return linkable

try:

# Hard linking is possible between two folders on the same device

return os.stat(source_dir).st_dev == os.stat(target_dir).st_devsrc = os.path.join(source_dir, "__nightly_test__")

except OSError:

# hard-linking between non-existing/non-accessible directories is not possible

return False

Sorry for taking a long time to get back to this. I don't believe that using stat to test the device is semantically correct here. We want to test if we can link, so the the way to do that is by trying to link a temporary file. The implementation details about which devices allow linking to which other devices isn't a strong enough guarantee that linking can happen, though it may be necessary based on the system

Good point. On the other hand, existing implementation does not, strictly speaking, answer the question whether files between two folders can be linked, because it will return false if user is not allowed to create files in source folder or if __nightly_test__ already exists in source folder or if __nightky_test__ exists in destination folder.
Considering all that, I think ideal approach would be to just try hard_link and if it fails to copy as suggested in https://github.com/pytorch/pytorch/pull/43771/files#r479539719

malfet

Please consider simpler implementation of _can_link, or perhaps just fall back to try: except: between link_file and copy_file

malfet · 2020-08-28T21:20:42Z

tools/nightly.py

+    if _can_link(platform, source_dir, target_dir):
        _link_files(listing, source_dir, target_dir)
+    else:
+        _copy_files(listing, source_dir, target_dir)


Wouldn't try: except: paradigm be better than if: then: in this case?

Suggested change

if _can_link(platform, source_dir, target_dir):

_link_files(listing, source_dir, target_dir)

else:

_copy_files(listing, source_dir, target_dir)

try::

_link_files(listing, source_dir, target_dir)

except OSError:

_copy_files(listing, source_dir, target_dir)

The try is in the _can_link() function. There is nothing particularly exceptional about choosing which function should be used to transfer files.

This approach eliminate the need of _can_link function completely, isn't it?

ezyang · 2020-09-18T18:14:34Z

@malfet are you satisfied by the response here, or is there more stuff we have to do? It would be good to move this along.

malfet · 2020-09-18T18:19:05Z

tools/nightly.py

+    if _can_link(platform, source_dir, target_dir):
        _link_files(listing, source_dir, target_dir)
+    else:
+        _copy_files(listing, source_dir, target_dir)


This approach eliminate the need of _can_link function completely, isn't it?

malfet · 2020-09-18T18:29:25Z

tools/nightly.py

+    src = os.path.join(source_dir, "__nightly_test__")
+    trg = os.path.join(target_dir, "__nightky_test__")
+    try:
+        with open(src, "w"):
+            pass
+        os.link(src, trg)
+        linkable = True
+    except OSError:
+        linkable = False
+    finally:
+        if os.path.isfile(trg):
+            os.remove(trg)
+        if os.path.isfile(src):
+            os.remove(src)
+    return linkable


Good point. On the other hand, existing implementation does not, strictly speaking, answer the question whether files between two folders can be linked, because it will return false if user is not allowed to create files in source folder or if __nightly_test__ already exists in source folder or if __nightky_test__ exists in destination folder.
Considering all that, I think ideal approach would be to just try hard_link and if it fails to copy as suggested in https://github.com/pytorch/pytorch/pull/43771/files#r479539719

scopatz · 2020-09-18T20:47:43Z

OK, here is a version that works with try-except and removes the _can_link() function

dr-ci · 2020-09-18T21:13:12Z

💊 CI failures summary and remediations

As of commit 677b919 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

2/2 broken upstream at merge base 6d31213 on Sep 18 from 11:38am to 4:20pm PDT (13 commits; f175830 - 40c09cf)

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:

pytorch_windows_vs2019_py36_cuda10.1_test2 since Sep 18
- 🔁 rerun

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_windows_vs2019_py36_cuda10.1_test1 on Sep 18 from 11:38am to 4:20pm PDT (13 commits; f175830 - 40c09cf)
- 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 2 times.

malfet

Thank you for the fix!

facebook-github-bot

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-21T20:17:04Z

@malfet merged this pull request in 7de512c.

nightly robustness fixes for linking across devices

e85ac88

scopatz mentioned this pull request Aug 28, 2020

nightly.py error #43761

Closed

pytorchbot added the open source label Aug 28, 2020

fixed dir

cd279b0

ezyang reviewed Aug 28, 2020

View reviewed changes

malfet reviewed Aug 28, 2020

View reviewed changes

malfet requested changes Aug 28, 2020

View reviewed changes

ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 31, 2020

malfet requested changes Sep 18, 2020

View reviewed changes

scopatz added 2 commits September 18, 2020 13:41

Merge branch 'master' into nightly-ln

3337956

rm _can_link()

677b919

malfet approved these changes Sep 21, 2020

View reviewed changes

facebook-github-bot reviewed Sep 21, 2020

View reviewed changes

facebook-github-bot closed this in 7de512c Sep 21, 2020

facebook-github-bot added the merged label Sep 21, 2020

scopatz deleted the nightly-ln branch September 23, 2020 15:53

mruberry added the Merged label Oct 28, 2020

nightly robustness fixes for linking across devices #43771

nightly robustness fixes for linking across devices #43771

Uh oh!

Conversation

scopatz commented Aug 28, 2020

Uh oh!

codecov bot commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Aug 28, 2020

Uh oh!

scopatz commented Aug 28, 2020

Uh oh!

malfet Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Sep 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scopatz commented Sep 18, 2020

Uh oh!

dr-ci bot commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🚧 1 ongoing upstream failure:

🚧 1 fixed upstream failure:

Uh oh!

malfet left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Aug 28, 2020 •

edited

Loading

malfet Aug 28, 2020 •

edited

Loading

dr-ci bot commented Sep 18, 2020 •

edited

Loading