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

Jump to bottom

Update protobuf to 3.13.1 #62571

Closed
wants to merge 3 commits into from
Closed

Update protobuf to 3.13.1 #62571

wants to merge 3 commits into from

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Aug 2, 2021 •
edited

Update bazel to 4.10.0

Update ASAN_SYMBOLIZER_PATH to llvm-7
Suppress vptr ubsan violations in test_jit
Fix ProtoBuf patching for ONNX which caused Windows builds to crash while attempting to free std::string allocated on stack

Fixes #62569
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Aug 2, 2021 •
edited

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 4ea4746 (more details on the Dr. CI page):

  • 2/2 failures possibly* introduced in this PR
    • 1/2 non-scanned failure(s)

1 failure not recognized by patterns:

Job Step Action
CircleCI pytorch_linux_xenial_py3_clang7_asan_test2 Run tests 🔁 rerun

1 job timed out:

  • pytorch_linux_xenial_py3_clang7_asan_test2

ci.pytorch.org: 1 failed

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@malfet malfet requested a review from Aug 2, 2021
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Aug 2, 2021

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

@garymm
Copy link
Collaborator

@garymm garymm commented Aug 3, 2021

Thanks for this change! The title of the PR is wrong. Should be "3.13.1", not "4.13.1".

@garymm garymm mentioned this pull request Aug 3, 2021
Closed
@malfet malfet changed the title Update protobuf to 4.13.1 Update protobuf to 3.13.1 Aug 3, 2021
@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from 5cf8da6 to ac6f0e1 Aug 4, 2021
walterddr
walterddr reviewed Aug 4, 2021
View changes
Copy link
Contributor

@walterddr walterddr left a comment

not sure why it broke these other test though :-(

@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from ac6f0e1 to d843da5 Aug 4, 2021
@malfet
Copy link
Contributor Author

@malfet malfet commented Aug 4, 2021

@walterddr docker builds are broken due to #62738, but big question I have is whether clang update will fix false positive in asan testing

@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from d843da5 to 04a28b5 Aug 5, 2021
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Aug 5, 2021

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

@malfet
Copy link
Contributor Author

@malfet malfet commented Aug 6, 2021

For some reason, after protobuf update it starts to crash on Windows for something as simple as:

python -c "import torch;import io;torch.onnx.export(torch.nn.Linear(3,3), (torch.rand(3, 3), ), io.BytesIO())"

malfet added 2 commits Aug 17, 2021
@malfet
Update protobuf to 4.13.1
b6345aa 
Update bazel to 4.10.0

Fixes #62569
@malfet
Update symbolizer path
cab464d 
Suppress vptr ubsan violation in test_jit
@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from 04a28b5 to cab464d Aug 17, 2021
@pytorch-probot pytorch-probot bot assigned pytorchbot and unassigned pytorchbot Aug 17, 2021
@pytorch-probot
Copy link

@pytorch-probot pytorch-probot bot commented Aug 17, 2021 •
edited

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/pytorch/pytorch/blob/4ea474605a8bc1615586109e87a8c2c80dc2c4b5/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default

Workflows Labels (bold enabled) Status
Triggered Workflows
linux-bionic-py3.8-gcc9-coverage ciflow/default ✅ triggered
linux-xenial-py3.6-gcc5.4 ciflow/default ✅ triggered
linux-xenial-py3.6-gcc7-bazel-test ciflow/default ✅ triggered
win-vs2019-cpu-py3 ciflow/default ✅ triggered
win-vs2019-cuda10.1-py3 ciflow/default ✅ triggered
Skipped Workflows
linux-xenial-cuda10.2-py3.6-gcc7 ciflow/slow 🚫 skipped
periodic-libtorch-linux-xenial-cuda11.3-py3.6-gcc7 ciflow/scheduled 🚫 skipped
periodic-linux-xenial-cuda11.3-py3.6-gcc7 ciflow/scheduled 🚫 skipped
periodic-win-vs2019-cuda11.3-py3 ciflow/scheduled 🚫 skipped

You can add a comment to the PR and tag @pytorchbot with the following commands: 
# ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun

# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow

For more information, please take a look at the CI Flow Wiki.

@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from db9b33c to b50efa1 Aug 18, 2021
@pytorch-probot pytorch-probot bot assigned pytorchbot and unassigned pytorchbot Aug 18, 2021
@malfet
Copy link
Contributor Author

@malfet malfet commented Aug 18, 2021 •
edited

Finally have a backtrace and a theory of what is going on:
image

[Edit]: Original hypothesis proved to be wrong, it's a good old duplicated symbols

@malfet
Copy link
Contributor Author

@malfet malfet commented Aug 19, 2021

image
Crash actually happens in ValueInfoProto::SharedDtor(), where an attempt is made to free empty string allocated on stack, because GetEmptyStringAlreadyInited() can return different values depending on the library it's called from... See code below

void ValueInfoProto::SharedDtor() {
  GOOGLE_DCHECK(GetArena() == nullptr);
  name_.DestroyNoArena(&::PROTOBUF_NAMESPACE_ID::internal::GetEmptyStringAlreadyInited());
  doc_string_.DestroyNoArena(&::PROTOBUF_NAMESPACE_ID::internal::GetEmptyStringAlreadyInited());
  if (this != internal_default_instance()) delete type_;
}

Though why it only happens on Windows is still a mystery to me.

@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from b50efa1 to 111c308 Aug 19, 2021
@malfet malfet force-pushed the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch from 111c308 to 4ea4746 Aug 19, 2021
@pytorch-probot pytorch-probot bot assigned pytorchbot and unassigned pytorchbot Aug 19, 2021
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Aug 19, 2021

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

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Aug 20, 2021

@malfet merged this pull request in bec75da.

@malfet malfet deleted the malfet/update-protobuf-to-3.13.1-bazel-to-4.10 branch Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterddr walterddr

Assignees
No one assigned
Labels
ciflow/default cla signed Merged
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

PyTorch Bazel scripts are incompatible with bazel-4.x
5 participants
@malfet @facebook-github-bot @garymm @walterddr @pytorchbot