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

protobuf: protobuf_25 -> protobuf_28 #338885

Merged
merged 6 commits into from
Sep 7, 2024

Conversation

GaetanLepage
Copy link
Contributor

@GaetanLepage GaetanLepage commented Sep 1, 2024

Description of changes

Bump default protobuf from 25.4 to 28.0, as well as the corresponding python package.

Changelog: https://github.com/protocolbuffers/protobuf/releases/tag/v28.0

Diff: protocolbuffers/protobuf@v25.4...v28.0

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Bump default protobuf from 25.4 to 28.0, as well as the corresponding
python package.

Changelog:
https://github.com/protocolbuffers/protobuf/releases/tag/v28.0

Diff: protocolbuffers/protobuf@v25.4...v28.0
@GaetanLepage GaetanLepage force-pushed the protobuf28 branch 2 times, most recently from ce01756 to b8122f5 Compare September 2, 2024 09:04
@GaetanLepage GaetanLepage marked this pull request as draft September 2, 2024 10:48
@GaetanLepage
Copy link
Contributor Author

protobuf.tests build fine on all four major platforms.

gaetan in 🌐 cuda in nixpkgs on  protobuf28 [$] 
❯ nix-build -A protobuf.tests --system x86_64-linux
/nix/store/41d62gsd7xl78pj11qlfp9k909d6f88d-grpc-1.62.1
/nix/store/bykqlv0bjv0zyzywwhz68pp55nkp0r9p-python3.12-protobuf-5.28.0

gaetan in 🌐 cuda in nixpkgs on  protobuf28 [$] 
❯ nix-build -A protobuf.tests --system aarch64-linux
/nix/store/2kxcwsp93b5p1hzg8g3rg16y75q09lq8-grpc-1.62.1
/nix/store/swi04sfdifiq9fykblzlfi9ddkc4dz4d-python3.12-protobuf-5.28.0

gaetan in 🌐 cuda in nixpkgs on  protobuf28 [$] 
❯ nix-build -A protobuf.tests --system x86_64-darwin
/nix/store/lnqpvjb3iw65j7m9axmg9isyjzfimhzp-grpc-1.62.1
/nix/store/sbgqsvpwfv7lif778prczj7s723gnq78-python3.12-protobuf-5.28.0

gaetan in 🌐 cuda in nixpkgs on  protobuf28 [$] 
❯ nix-build -A protobuf.tests --system aarch64-darwin
/nix/store/ylqxhwxjazxdzxql0ir5lp2v0wbwfvfb-grpc-1.62.1
/nix/store/0wrpqgp58nrbmzvipqddq0c6pj2b3gcd-python3.12-protobuf-5.28.0

@SuperSandro2000
Copy link
Member

I don't think we get around building some more packages. Maybe we can just grep for users of protobuf and grab what seems to be important?

@GaetanLepage
Copy link
Contributor Author

I was also able to build jax(-lib) with and without cuda support on both linux platforms.

@ConnorBaker
Copy link
Contributor

I’ll try building and testing PyTorch later today to make sure that works (might be an issue, see https://discuss.pytorch.org/t/protobuf-versions-for-pytorch-2-1-2/199256).

@ConnorBaker
Copy link
Contributor

Well, I was able to build and run nix-cuda-test (https://github.com/connorbaker/nix-cuda-test):

Seed set to 42
Using bfloat16 Automatic Mixed Precision (AMP)
GPU available: True (cuda), used: True
TPU available: False, using: 0 TPU cores
HPU available: False, using: 0 HPUs
Files already downloaded and verified
Files already downloaded and verified
LOCAL_RANK: 0 - CUDA_VISIBLE_DEVICES: [0]

  | Name      | Type             | Params | Mode 
-------------------------------------------------------
0 | criterion | CrossEntropyLoss | 0      | train
1 | model     | ViT              | 86.3 M | train
-------------------------------------------------------
86.3 M    Trainable params
0         Non-trainable params
86.3 M    Total params
345.317   Total estimated model params size (MB)
130       Modules in train mode
0         Modules in eval mode
Epoch 9: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 781/781 [01:26<00:00,  9.07it/s, v_num=9, train_loss=2.350, val_loss=2.330]`Trainer.fit` stopped: `max_epochs=10` reached.                                                                                                                                                                                                                
Epoch 9: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 781/781 [01:28<00:00,  8.83it/s, v_num=9, train_loss=2.350, val_loss=2.330]

So that's a good sign.

@GaetanLepage
Copy link
Contributor Author

Well, I was able to build and run nix-cuda-test (https://github.com/connorbaker/nix-cuda-test):

Seed set to 42
Using bfloat16 Automatic Mixed Precision (AMP)
GPU available: True (cuda), used: True
TPU available: False, using: 0 TPU cores
HPU available: False, using: 0 HPUs
Files already downloaded and verified
Files already downloaded and verified
LOCAL_RANK: 0 - CUDA_VISIBLE_DEVICES: [0]

  | Name      | Type             | Params | Mode 
-------------------------------------------------------
0 | criterion | CrossEntropyLoss | 0      | train
1 | model     | ViT              | 86.3 M | train
-------------------------------------------------------
86.3 M    Trainable params
0         Non-trainable params
86.3 M    Total params
345.317   Total estimated model params size (MB)
130       Modules in train mode
0         Modules in eval mode
Epoch 9: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 781/781 [01:26<00:00,  9.07it/s, v_num=9, train_loss=2.350, val_loss=2.330]`Trainer.fit` stopped: `max_epochs=10` reached.                                                                                                                                                                                                                
Epoch 9: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 781/781 [01:28<00:00,  8.83it/s, v_num=9, train_loss=2.350, val_loss=2.330]

So that's a good sign.

Thanks for testing @ConnorBaker !

@SuperSandro2000, any remaining blocker/assertion before merging this ?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 6, 2024
@GaetanLepage
Copy link
Contributor Author

I plan to merge this later today.
Please, do not hesitate to express any doubt/feedback.

As this will land in staging we will have some time to catch catastrophic regressions before it gets to master.

@GaetanLepage GaetanLepage merged commit f4baf46 into NixOS:staging Sep 7, 2024
26 of 27 checks passed
@GaetanLepage GaetanLepage deleted the protobuf28 branch September 7, 2024 15:14
@SomeoneSerge
Copy link
Contributor

Thank you @GaetanLepage

@trofi
Copy link
Contributor

trofi commented Sep 8, 2024

Bisect says 7cdaa0b protobuf: protobuf_25 -> protobuf_28 broke protobufc build in staging as:

$ nix build --no-link github:NixOS/nixpkgs/staging#protobufc -L
...
protobuf-c> Running phase: buildPhase
protobuf-c> build flags: SHELL=/nix/store/b4vbmd1dknwxvzm5sswcd87w4a76wdsm-bash-5.2p32/bin/bash
protobuf-c>   GEN      protobuf-c/protobuf-c.pb.cc
protobuf-c>   CXX      protoc-c/protoc_gen_c-c_bytes_field.o
protobuf-c> In file included from protoc-c/c_bytes_field.cc:64:
protobuf-c> ./protoc-c/c_helpers.h: In function 'int google::protobuf::compiler::c::FieldSyntax(const google::protobuf::FieldDescriptor*)':
protobuf-c> ./protoc-c/c_helpers.h:179:46: error: 'class google::protobuf::FileDescriptorLegacy' has no member named 'syntax'
protobuf-c>   179 |   return FileDescriptorLegacy(field->file()).syntax() == FileDescriptorLegacy::SYNTAX_PROTO3 ? 3 : 2;
protobuf-c>       |                                              ^~~~~~
protobuf-c> ./protoc-c/c_helpers.h:179:80: error: 'SYNTAX_PROTO3' is not a member of 'google::protobuf::FileDescriptorLegacy'
protobuf-c>   179 |   return FileDescriptorLegacy(field->file()).syntax() == FileDescriptorLegacy::SYNTAX_PROTO3 ? 3 : 2;
protobuf-c>       |                                                                                ^~~~~~~~~~~~~
protobuf-c> make: *** [Makefile:1567: protoc-c/protoc_gen_c-c_bytes_field.o] Error 1

@GaetanLepage
Copy link
Contributor Author

Probably related to protobuf-c/protobuf-c#730.
Doesn't protobuf-c look unmaintained ? (no changes for the past 10 months)

@GaetanLepage
Copy link
Contributor Author

This issue is even closer: protobuf-c/protobuf-c#709
Maybe patching with the commits from protobuf-c/protobuf-c#711 could work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants