Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the model package's initialization file to export the MODEL_MAPPING dictionary. The review feedback points out that exposing this raw dictionary directly in the public API could lead to encapsulation issues and bypass validation logic, suggesting that the register_model function should be exported instead to maintain internal consistency.
| from .gpt_model import GPTModel | ||
| from .mm_gpt_model import MultimodalGPTModel | ||
| from .register import get_mcore_model, get_mcore_model_type, get_model_meta | ||
| from .register import MODEL_MAPPING, get_mcore_model, get_mcore_model_type, get_model_meta |
There was a problem hiding this comment.
Exposing the raw MODEL_MAPPING dictionary in the public API allows external code to modify the registry directly, which can bypass the validation logic in register_model and lead to inconsistencies with the internal model_type_mapping cache in register.py. If the intention is to allow external registration of models, it is recommended to export the register_model function instead. If the intention is to allow inspection of the registered models, consider providing a read-only view or a getter function to maintain better encapsulation.
| from .register import MODEL_MAPPING, get_mcore_model, get_mcore_model_type, get_model_meta | |
| from .register import get_mcore_model, get_mcore_model_type, get_model_meta, register_model |
No description provided.