Skip to content

Conversation

@ptovam
Copy link
Contributor

@ptovam ptovam commented Nov 18, 2025

This PR introduces get_total_num_hidden_layers() as a standalone helper in ModelConfig.
The logic previously lived inside get_layers_start_end_indices().

Motivation:
Expose the model’s total hidden-layer count through a clean, reusable API.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly refactors the logic for determining the total number of hidden layers into a new helper method, get_total_num_hidden_layers, within ModelConfig. This is a good change as it encapsulates complex, model-specific logic into a dedicated, reusable function, improving code clarity. The update to use a string forward reference for the ParallelConfig type hint is also a good practice to prevent potential circular imports. I have one suggestion to improve the maintainability of the newly extracted method.

Copy link
Member

@yewentao256 yewentao256 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work!

@yewentao256 yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 18, 2025
@ptovam ptovam force-pushed the get_total_num_hidden_layers branch from aea1ab0 to e84d214 Compare November 18, 2025 20:22
@ptovam
Copy link
Contributor Author

ptovam commented Nov 18, 2025

Thank @yewentao256!
Pushed a small update to align the type-hint style with the rest of the file (removed the string annotation).

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 19, 2025 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants