Proper deregistration of static types and objects#61290
Conversation
Contributor
|
Hi @dorozhkin-stc! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 607cf78 (more details on the Dr. CI page):
🕵️ 76 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
| Job | Step | Action |
|---|---|---|
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Test | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun | |
| Build | 🔁 rerun |
❄️ 1 failure tentatively classified as flaky
but reruns have not yet been triggered to confirm:
binary_linux_manywheel_3_8m_rocm4_0_1_devtoolset7_nightly_test (1/1)
Step: "Set Up CI Environment After attach_workspace" (full log | diagnosis details | 🔁 rerun) ❄️
E: Failed to fetch http://mirrors.lug.mtu.edu/u...50ab2bf45 404 Not Found [IP: 104.225.141.141 80]
+ sudo rm -f /etc/apt/heroku.list
+ sudo rm -f /etc/apt/openjdk-r-ubuntu-ppa-xenial.list
+ sudo rm -f /etc/apt/partner.list
+ sudo sed -i -e 's/http:\/\/.*archive/mirror:\/\/mirrors/' -e 's/\/ubuntu\//\/mirrors.txt/' /etc/apt/sources.list
+ sudo tee /etc/apt/apt.conf.d/80-retries
+ echo 'APT::Acquire::Retries "3";'
APT::Acquire::Retries "3";
+ retry sudo apt-get update -qq
+ sudo apt-get update -qq
W: --force-yes is deprecated, use one of the options starting with --allow instead.
E: Failed to fetch http://mirrors.lug.mtu.edu/ubuntu/dists/focal-updates/main/cnf/by-hash/SHA256/7a7c6356fd1a2cc7d470601f0f28bb4d8ffa42835dc39d19bcda52a50ab2bf45 404 Not Found [IP: 104.225.141.141 80]
E: Some index files failed to download. They have been ignored, or old ones used instead.
+ sudo apt-get update -qq
W: --force-yes is deprecated, use one of the options starting with --allow instead.
+ retry sudo apt-get -y install moreutils expect-dev
+ sudo apt-get -y install moreutils expect-dev
Reading package lists... 0%
Reading package lists... 100%
Reading package lists... Done
Building dependency tree... 0%
Building dependency tree... 0%
Building dependency tree... 50%
Building dependency tree... 50%
Building dependency tree
Reading state information... 0%
Reading state information... 0%
Reading state information... Done
Note, selecting 'expect' instead of 'expect-dev'
The following additional packages will be installed:
This comment was automatically generated by Dr. CI (expand for details).
Please report bugs/suggestions to the (internal) Dr. CI Users group.
Contributor
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
peterjc123
reviewed
Jul 22, 2021
Contributor
|
@jamesr66a do you know who the right person to review this is? |
Author
|
Rebased on recent master and cleaned up. Is there a windows build corresponding to my branch available online so I can double-check if the issue is fixed? |
Contributor
|
@dorozhkin-stc I went ahead and triggered binary builds for this PR, although I don't know if the binaries are saved in a spot you can get at (@zhouzhuojie do you know?) |
Collaborator
|
Does this have implications on custom class usage that should be mentioned in the documentation? |
Author
None known. The intended static deregistration on library unload (which is the reverse of the static registration on library load) is designed to run completely automatically and transparent to the user. |
Contributor
|
you can get the windows binaries on the "Artifacts" tab, see https://app.circleci.com/pipelines/github/pytorch/pytorch/354963/workflows/78d15278-8a89-47ad-96ee-2bbeeb3b8cb5/jobs/14962925/artifacts for example |
jamesr66a
approved these changes
Jul 26, 2021
Collaborator
jamesr66a
left a comment
There was a problem hiding this comment.
This seems fine to me. Adding @tugsbayasgalan @SplitInfinity @gmagogsfm as most proximate torchbind owners for additional review
Contributor
|
@jamesr66a will you land? |
Collaborator
|
@ezyang sure |
Collaborator
|
@dorozhkin-stc can you rebase? Some of the test jobs seem to be failing due to infrastructure errors |
Author
|
After rebase there are still some job failures. Seems unrelated though (hard drive is out of free space on Windows machine and deadline timeout on the others). |
gmagogsfm
approved these changes
Aug 7, 2021
Contributor
|
Looks good, the CI test failures seem unrelated to your change. Could you rebase to trigger a re-run so that real issues (if any) don't get masked by those unrelated failures? Thanks! |
Hopefully this fixes possible crash when unload torch_cpu on Windows. See #61111 for the details.
|
Is there an update on this? I would really like to use a newer version of libtorch, and I think this issue is preventing me. |
DBraun
pushed a commit
to DBraun/pytorch
that referenced
this pull request
Nov 1, 2021
Collaborator
|
@dorozhkin-stc it looks like there's still infra failures on CI here, can you rebase again? |
Contributor
|
@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Contributor
|
bumping |
Contributor
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
pytorchmergebot
pushed a commit
that referenced
this pull request
Jul 31, 2022
Trying to rebase #61290 into latest pytorch:master Pull Request resolved: #67632 Approved by: https://github.com/ezyang
jjsjann123
pushed a commit
to jjsjann123/nvfuser
that referenced
this pull request
Oct 29, 2022
Trying to rebase pytorch/pytorch#61290 into latest pytorch:master Pull Request resolved: pytorch/pytorch#67632 Approved by: https://github.com/ezyang
jjsjann123
pushed a commit
to jjsjann123/nvfuser
that referenced
this pull request
Nov 10, 2022
Trying to rebase pytorch/pytorch#61290 into latest pytorch:master Pull Request resolved: pytorch/pytorch#67632 Approved by: https://github.com/ezyang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out on Windows static destructror should run on behalf the same library which run static constructor. For example, the objects registered by
torch_cuda_cpp.dllshould be properly deregistered bytorch_cuda_cpp.dll, otherwisetorch_cpu.dllwill crash on unload.Hopefully fixes #61111