[go: nahoru, domu]

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

[submodule] update protobuf to tag v3.19.6 #95128

Closed
wants to merge 7 commits into from

Conversation

cyyever
Copy link
Collaborator
@cyyever cyyever commented Feb 19, 2023

Fixes #ISSUE_NUMBER

@pytorch-bot
Copy link
pytorch-bot bot commented Feb 19, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/95128

Note: Links to docs will display an error until the docs builds have been completed.

❌ 4 Failures

As of commit a8de144:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Feb 19, 2023
@cyyever cyyever marked this pull request as draft February 19, 2023 06:10
@cyyever cyyever changed the title [submodule] update protobuf to tag v22.0 [submodule] update protobuf to tag v21.12 Feb 19, 2023
@cyyever cyyever marked this pull request as ready for review February 19, 2023 13:28
@Skylion007
Copy link
Collaborator
Skylion007 commented Feb 19, 2023

I was going to get around to this at some point. You will also want to update the pip install pinned versions of protobuf to the 21.12 as well. (do a grep through the yaml and requirement files).

@Skylion007 Skylion007 added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 19, 2023
@vadimkantorov
Copy link
Contributor
vadimkantorov commented Feb 20, 2023

There is also this issue requiring protobuf < 3.2.0 for tensorboard to work: #90374. I propose there that torch.utils.tensorboard should not fail on import because of problematic protobuf dependencies

@cyyever
Copy link
Collaborator Author
cyyever commented Feb 20, 2023

I was going to get around to this at some point. You will also want to update the pip install pinned versions of protobuf to the 21.12 as well. (do a grep through the yaml and requirement files).

No pip dependency found after a grep. Bazel files were also checked but no specific versions hard-wired.

@cyyever
Copy link
Collaborator Author
cyyever commented Feb 20, 2023

There is also this issue requiring protobuf < 3.2.0 for tensorboard to work: #90374. I propose there that torch.utils.tensorboard should not fail on import because of problematic protobuf dependencies

May be we need to upgrade tensorboard dependency as well.

requirements.txt Outdated Show resolved Hide resolved
@vadimkantorov
Copy link
Contributor

May be we need to upgrade tensorboard dependency as well.

As far as I know, tensorboard still has this problem unfixed. So you could bump the min version of tensorboard, but it still wouldn't fix protobuf <3.2.0 requirement of tensorboard: tensorflow/tensorboard#6147 v3.19.6 seems to be the most recent supported version

@cyyever cyyever changed the title [submodule] update protobuf to tag v21.12 [submodule] update protobuf to tag v3.19.6 Feb 21, 2023
Copy link
Contributor
@malfet malfet left a comment

Choose a reason for hiding this comment

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

For those kinds of changes, I want some rigorous and reproducible test plan, like a script that dumps public symbols on all 3 platforms(with priority on Linux and Windows) and showing that no new ones were added.

Another good test to add (and may be to CI): Install torch + different version of protobuf from pipy and make sure they do not conflict with each other, as all the ugly patching that you are removing was done in order to make protobuf not leak any symbols externally.

@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 22, 2023
@github-actions
Copy link

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Jun 16, 2023
@github-actions github-actions bot closed this Jul 16, 2023
@cyyever cyyever deleted the protobuf branch May 30, 2024 09:47
@cyyever cyyever restored the protobuf branch May 30, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request open source Stale topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants