Skip to content

Fix supports_{tp/pp}_plan#44696

Merged
hmellor merged 4 commits intohuggingface:mainfrom
hmellor:fix-tp-pp-plan
Mar 18, 2026
Merged

Fix supports_{tp/pp}_plan#44696
hmellor merged 4 commits intohuggingface:mainfrom
hmellor:fix-tp-pp-plan

Conversation

@hmellor
Copy link
Member

@hmellor hmellor commented Mar 14, 2026

In configs, base_model_pp_plan and base_model_tp_plan default to None

In models, _pp_plan and _tp_plan look like they default to None based on the class variables, but will actually always be a dict because of post_init.

This means that supports_pp_plan and supports_tp_plan always return True.

This PR:

  • Fixes the supports methods
  • Adds some input validation to pp_plan's setter (same as tp_plan's)
  • Fixes the type hint for PreTrainedModel._pp_plan because it's always a tuple[str, str] not an Enum

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Member

@Cyrilvallez Cyrilvallez left a comment

Choose a reason for hiding this comment

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

I think we should add a checl for emptyness!

Comment on lines +4407 to +4408
# Check if model has a TP plan
if self._tp_plan:
Copy link
Member

Choose a reason for hiding this comment

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

Shiuldn't we check if it's empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was checking for truthiness because the default value is None if post_init is not called

Comment on lines +4428 to +4429
# Check if model has a PP plan
if self._pp_plan:
Copy link
Member

Choose a reason for hiding this comment

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

Same here, let's check if it's empty

Copy link
Member

@Cyrilvallez Cyrilvallez left a comment

Choose a reason for hiding this comment

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

Alright, we can use the implicit bool value of the dict

@hmellor hmellor enabled auto-merge March 16, 2026 20:46
@Cyrilvallez
Copy link
Member

@bot /style

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2026

Style fix bot fixed some files and pushed the changes.

@hmellor hmellor added this pull request to the merge queue Mar 18, 2026
Merged via the queue into huggingface:main with commit 9f93b61 Mar 18, 2026
28 checks passed
@hmellor hmellor deleted the fix-tp-pp-plan branch March 18, 2026 12:33
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