Skip to content

Read ps3_scales, not s3_scales, from the PS3 vision encoder - #287

Open
appleweiping wants to merge 1 commit into
NVlabs:mainfrom
appleweiping:fix/ps3-scales-282
Open

Read ps3_scales, not s3_scales, from the PS3 vision encoder#287
appleweiping wants to merge 1 commit into
NVlabs:mainfrom
appleweiping:fix/ps3-scales-282

Conversation

@appleweiping

Copy link
Copy Markdown

Fixes #282.

PS3VisionEncoder in ps3-torch defines ps3_scales; there is no s3_scales attribute on it, so every access of the misspelled name raises AttributeError as soon as a PS3 model is built — config.ps3=True fails before training or inference can start.

#282 reports the occurrence in multimodal_encoder/builder.py. The same misspelling occurs four more times in llava_arch.py's encode_images_ps3(), on the path reached when ps3_dynamic_aspect_ratio is enabled, so this PR fixes all five.

Two tests are included that run without torch or ps3-torch installed: one builds a PS3 vision tower against a stubbed encoder and asserts mm_scale_num is derived from ps3_scales; the other scans the source for the misspelling, which keeps the four dynamic-aspect-ratio occurrences covered without needing a GPU. The scan matches .s3_scales specifically and leaves the unrelated s2_scales used by the dynamic-S2 path alone. Both tests fail against main and pass with this change.

I have not been able to run a full PS3 model end-to-end (no GPU or ps3-torch in my environment); verification is against the ps3-torch source plus the tests above.

🤖 Generated with Claude Code

`PS3VisionEncoder` in ps3-torch defines `ps3_scales`; it has no `s3_scales`
attribute. Every access of the misspelled name raises AttributeError as soon as
a PS3 model is built, so `config.ps3=True` fails before training or inference
can start.

The name occurs five times:

- `multimodal_encoder/builder.py` in `build_vision_tower()`, reached whenever
  `config.ps3` is set. This is the occurrence reported in NVlabs#282.
- `llava_arch.py` in `encode_images_ps3()`, four times, reached when
  `ps3_dynamic_aspect_ratio` is enabled.

Adds two tests that run without torch or ps3-torch installed: one builds a PS3
vision tower against a stubbed encoder and asserts `mm_scale_num` is derived
from `ps3_scales`, and one scans the source for the misspelling so the four
occurrences on the dynamic-aspect-ratio path stay covered too. The scan matches
`.s3_scales` specifically, leaving the unrelated `s2_scales` used by the
dynamic-S2 path alone. Both fail against the current code and pass with this
change.

Fixes NVlabs#282

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@appleweiping

Copy link
Copy Markdown
Author

Validated the latest head b408d23 locally: pytest -q tests/test_multimodal_encoder_builder.py -> 2 passed. The tests cover the ps3_scales attribute and source-level protection against the old s3_scales typo without requiring torch/PS3/GPU. The five typo fixes are ready for review.

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.

Fix typo: s3_scales should be ps3_scales in multimodal_encoder/builder.py (AttributeError)

1 participant