-
Notifications
You must be signed in to change notification settings - Fork 116
Update for transformers 5.0 #1436
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
base: main
Are you sure you want to change the base?
Update for transformers 5.0 #1436
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThis pull request relaxes transformers version constraints across multiple projects, introduces weight initialization and state management improvements for TransformerEngine-backed models, updates weight tying declarations from tuples to typed class variable mappings, and adds TCP port fixture infrastructure to distributed tests for rendezvous configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6b22f73 to
d9c65e7
Compare
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
20b3b6f to
af11872
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bionemo-recipes/models/esm2/src/esm/convert.py (1)
122-135:⚠️ Potential issue | 🟡 MinorRemove redundant
post_init()call;apply_transforms()already callstie_weights().Testing confirms the current code is safe—weight values are preserved correctly through the
post_init()call (test_convert.py validates roundtrip conversion with strict tolerances). However,apply_transforms()already invokestie_weights()before returning, making the subsequentpost_init()call redundant. Also note thatconvert_esm_hf_to_te()does not callpost_init(), creating an inconsistency.Suggested fix
- output_model.post_init()bionemo-recipes/models/esm2/tests/conftest.py (1)
16-43:⚠️ Potential issue | 🟡 MinorAdd a second blank line after the import block.
Ruff/isort will flag a single blank line before the module-level TRITON env block.
🧹 Suggested fix
-from esm.convert import convert_esm_hf_to_te - -# Fix Triton UTF-8 decoding issue by setting CUDA library path +from esm.convert import convert_esm_hf_to_te + + +# Fix Triton UTF-8 decoding issue by setting CUDA library pathAs per coding guidelines, Follow import sorting configuration as per isort with 2 lines after imports.
🧹 Nitpick comments (3)
bionemo-recipes/models/amplify/pyproject.toml (1)
21-21: Confirm whether AMPLIFY should stay capped at Transformers <5.0.If AMPLIFY is now v5-compatible, aligning this constraint (e.g.,
transformers>=5.0,<6) would prevent users from staying on v4. If not, the TODO is fine but this will keep the package incompatible with v5 installs.💡 Optional alignment with v5 support
- "transformers<5.0", # TODO(BIO-143): update AMPLIFY to support Transformers v5 + "transformers>=5.0,<6", # TODO(BIO-143): update AMPLIFY to support Transformers v5bionemo-recipes/recipes/geneformer_native_te_mfsdp_fp8/requirements.txt (1)
9-9: Add an explicit v5 lower bound to avoid silently running v4.An unconstrained
transformerscan be satisfied by an already-installed v4, which defeats the v5 migration and risks runtime mismatches. Considertransformers>=5.0,<6to lock the supported major.♻️ Suggested constraint
-transformers +transformers>=5.0,<6bionemo-recipes/recipes/esm2_native_te/requirements.txt (1)
11-11: Consider boundingtransformersto the v5 major line.Leaving this unbounded can allow v6+ upgrades with breaking changes. A constrained range still permits v5 updates while preventing accidental major bumps.
🔧 Suggested constraint
-transformers +transformers>=5.0,<6.0
Currently blocked by huggingface/transformers#43510
Summary by CodeRabbit
New Features
Improvements