Skip to content

fix(models): Rename fpn_position_encoding to fpn_position_embeddings#43462

Closed
lllangWV wants to merge 1 commit intohuggingface:mainfrom
lllangWV:sam3video_inconsistncy
Closed

fix(models): Rename fpn_position_encoding to fpn_position_embeddings#43462
lllangWV wants to merge 1 commit intohuggingface:mainfrom
lllangWV:sam3video_inconsistncy

Conversation

@lllangWV
Copy link

What does this PR do?

The dataclass output attribute was named fpn_position_encoding but code in _prepare_vision_features was accessing fpn_position_embeddings, causing an AttributeError. Renamed to fpn_position_embeddings across all SAM2, SAM3, and EdgeTAM models for consistency with transformers naming conventions.

Affected models: sam2, sam2_video, sam3, sam3_tracker, sam3_tracker_video, sam3_video, edgetam, edgetam_video

Maybe the other changes where unnecessary, but I like to have consistent naming scheme through similar modules.

I could have just modified

transformers/models/sam3_tracker_video/modeling_sam3_tracker_video.py", line 1912 to

vision_pos_embeds = image_outputs.fpn_position_embeddings 

->

vision_pos_embeds = image_outputs.fpn_position_encodings

If you all just want the minimal change I can go back and undo the other changes

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • I ran doc-builder build transformers docs/source/en --build_dir ~/tmp/test-build Nothing seemed to change. I think the docs for this get automatically updated.
Building docs for transformers docs/source/en /home/lllang/tmp/test-build/transformers/main/en
Building the MDX files:  13%|████████████████████████                                                                                                                                                                | 81/618 [00:00<00:00, 797.15it/s]use_kernel_func_from_hub is not available in the installed kernels version. Please upgrade kernels to use this feature.
Building the MDX files: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 618/618 [00:47<00:00, 13.12it/s]
Resolving internal links: 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 605/605 [00:01<00:00, 584.87it/s]
  • [ x] Did you write any new necessary tests?
    • Due to the name change, a test had to be modified tests/models/sam3/test_modeling_sam3.py

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

The dataclass output attribute was named `fpn_position_encoding` but
code in `_prepare_vision_features` was accessing `fpn_position_embeddings`,
causing an AttributeError. Renamed to `fpn_position_embeddings` across
all SAM2, SAM3, and EdgeTAM models for consistency with transformers
naming conventions.

Affected models: sam2, sam2_video, sam3, sam3_tracker, sam3_tracker_video,
sam3_video, edgetam, edgetam_video
@github-actions
Copy link
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: edgetam, edgetam_video, sam2, sam2_video, sam3, sam3_tracker, sam3_tracker_video, sam3_video

@Rocketknight1
Copy link
Member

cc @NielsRogge @yonigozlan @molbap

@tomaarsen tomaarsen self-assigned this Jan 26, 2026
@tomaarsen
Copy link
Member

Hello!

This was taken care of with #43487, which instead renames fpn_position_embeddings (which was erroneously introduced in #42564 3 days ago) to fpn_position_encoding, which was originally intended. Thank you for your PR, but the issue has now been resolved.

  • Tom Aarsen

@tomaarsen tomaarsen closed this Jan 26, 2026
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.

3 participants