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

Pin protobuf<4 #543

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Pin protobuf<4 #543

merged 3 commits into from
Jun 8, 2022

Conversation

ashnair1
Copy link
Collaborator

@ashnair1 ashnair1 commented May 26, 2022

Tests were failing due to the recent release of protobuf 4.21.0. See protocolbuffers/protobuf#10051

Error propagation: protobuf > tensorboard > pytorch_lightning > torchgeo

adamjstewart
adamjstewart previously approved these changes May 26, 2022
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Thanks! Hopefully we can remove this soon.

@adamjstewart adamjstewart enabled auto-merge (squash) May 26, 2022 18:41
@adamjstewart
Copy link
Collaborator

Looks like 4.21.0 was yanked so maybe the protobuf devs understand that this was a bad idea: https://pypi.org/project/protobuf/#history

If you rerun the tests on your other PR it might pass without needing this PR. Not sure if protobuf or tensorboard will get a new release first.

@ashnair1
Copy link
Collaborator Author

ashnair1 commented May 26, 2022

Not sure what's going on with this test failure. I could add the protobuf entry under pip in the environment.yml but I'm not sure that's the issue here.

Per the discussion in the linked issue, it might be a good idea to pin protobuf=3.21.1 since it seems the devs still plan to push a 4.20.1

@adamjstewart
Copy link
Collaborator

adamjstewart commented May 26, 2022

Yeah, definitely pinning to <4 is a good idea until our deps fix themselves. This issue affects just about every library that depends on protobuf (tensorboard, torch, and likely others).

auto-merge was automatically disabled May 26, 2022 19:30

Head branch was pushed to by a user without write access

@ashnair1 ashnair1 changed the title Pin protobuf<4.21.0 Pin protobuf<4 May 26, 2022
@adamjstewart
Copy link
Collaborator

I also have no idea why the Windows tests are failing. We don't use environment.yml in those tests so it can't be related to that.

@adamjstewart
Copy link
Collaborator

Can you rebase and also add this to requirements.txt?

@ashnair1 ashnair1 force-pushed the protobuf-pin branch 2 times, most recently from 37b60d8 to 0366c23 Compare June 6, 2022 18:01
adamjstewart
adamjstewart previously approved these changes Jun 6, 2022
@adamjstewart adamjstewart enabled auto-merge (squash) June 6, 2022 18:03
auto-merge was automatically disabled June 6, 2022 18:58

Head branch was pushed to by a user without write access

adamjstewart
adamjstewart previously approved these changes Jun 6, 2022
@ashnair1
Copy link
Collaborator Author

ashnair1 commented Jun 6, 2022

Ran into a failed coverage test (no idea why that happened). Pushed again to see if error reappears.

@adamjstewart
Copy link
Collaborator

No idea what's going on with coverage. You have all the same commits as main, so it's not like new code has been added and the absolute number of lines of coverage have changed...

Also, you'll probably want to add protobuf to the ignore list in .github/dependabot.yml so that dependabot doesn't try to update it.

There is a new release of protobuf now that hasn't been yanked, but there's also a new release of pytorch-lightning that pins protobuf to an older version, so we're technically good for now. PyTorch has also pinned to older protobuf and 1.12.0 seems poised for a release soon. Hopefully we won't need this for long.

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Jun 8, 2022

There is a new release of protobuf now that hasn't been yanked, but there's also a new release of pytorch-lightning that pins protobuf to an older version, so we're technically good for now. PyTorch has also pinned to older protobuf and 1.12.0 seems poised for a release soon. Hopefully we won't need this for long.

Wouldn't we then need to update the min version of pytorch-lightning and/or pytorch? Seems like a big jump from 1.3 to 1.6.5 and 1.7.0 to 1.12.0 respectively.

@adamjstewart
Copy link
Collaborator

No need to change the minimum versions, TorchGeo is still compatible with both the older and newer releases. Someone might need to manually install an older version of protobuf to get PyTorch or tensorboard working, but that's not a TorchGeo issue.

@adamjstewart adamjstewart merged commit aec783d into microsoft:main Jun 8, 2022
@ashnair1 ashnair1 deleted the protobuf-pin branch June 9, 2022 05:55
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants