fix: propagate fields in composite ModelCard synthesis#29
fix: propagate fields in composite ModelCard synthesis#29zubatyuk wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Greptile SummaryThis PR fixes Important Files Changed
Reviews (4): Last reviewed commit: "fix(test): harden ModelCard boolean-fiel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| bool_fields = { | ||
| name | ||
| for name, info in ModelCard.model_fields.items() | ||
| if info.annotation is bool | ||
| } |
There was a problem hiding this comment.
info.annotation is bool
FieldInfo.annotation for Annotated[bool, Field(...)] fields is currently unwrapped to the bare bool type by Pydantic v2, so info.annotation is bool works today. However, this is an undocumented internal detail. If Pydantic changes this behaviour in a future major version, or if a future contributor adds a ModelCard boolean field using a slightly different annotation style (e.g. bool | None), the set comprehension could silently produce an incorrect bool_fields, causing the completeness assertion to fail with a confusing message.
A more explicit and forward-compatible approach is to unwrap Annotated yourself:
| bool_fields = { | |
| name | |
| for name, info in ModelCard.model_fields.items() | |
| if info.annotation is bool | |
| } | |
| import typing | |
| bool_fields = { | |
| name | |
| for name, info in ModelCard.model_fields.items() | |
| if ( | |
| info.annotation is bool | |
| or ( | |
| typing.get_origin(info.annotation) is typing.Annotated | |
| and typing.get_args(info.annotation)[0] is bool | |
| ) | |
| ) | |
| } |
This makes the intent explicit and handles both plain bool and Annotated[bool, ...] fields without relying on Pydantic's internal unwrapping.
|
/ok to test d608b78 |
…elCard synthesis Signed-off-by: Roman Zubatyuk <rzubatiuk@nvidia.com>
… types The policy completeness test used `info.annotation is bool` to discover ModelCard boolean fields. This relies on Pydantic v2 internally unwrapping Annotated types, which is undocumented. Add an explicit check for `Annotated[bool, ...]` so the test remains correct if a future ModelCard field uses `Annotated[bool, Field(...)]`. Signed-off-by: Roman Zubatyuk <rzubatiuk@nvidia.com>
zubatyuk
force-pushed
the
fix/composite-model-card-synthesis
branch
from
d608b78 to
c514feb
Compare
2 participants
ALCHEMI Toolkit Pull Request
Description
ComposableModelWrapper._build_model_card()only passed 7 of 16ModelCardboolean fields to the synthesized card. The remaining fields silently defaulted toFalse, makingwrapper.input_data()andwrapper.model_cardincorrect for composites that include charge-requiring sub-models like PME or Ewald.Type of Change
Related Issues
None.
Changes Made
needs_node_chargesandneeds_system_chargeswithany()aggregation in_build_model_card(), so the composite correctly reports input requirements from sub-models.includes_dispersionandincludes_long_range_electrostaticswithany()aggregation, so the composite card reflects physics metadata that was already read for double-counting warnings but never written to the synthesized card.supports_node/edge/graph_embeddings,supports_hessians, andsupports_dipolesare intentionally keptFalseon composites.examples/advanced/07_composable_model_composition.pyto accurately describe the composite card as "aggregated sub-model requirements, additive capabilities, and metadata" rather than "union of sub-model capabilities."Testing
make pytest)make lint)New tests in
test/models/test_composable.py::TestModelCardSynthesis:test_needs_node_charges_is_any/test_needs_node_charges_false_when_none_need/test_needs_system_charges_is_any— verifyneeds_*aggregation.test_input_data_includes_node_charges/test_input_data_includes_graph_charges— integration tests verifyingwrapper.input_data()includes charge fields when a sub-model requires them.test_includes_dispersion_propagated/test_includes_long_range_electrostatics_propagated— verifyincludes_*metadata propagation.test_embedding_support_stays_false— negative test confirming embedding support is not propagated even when a child model supports it.test_model_card_policy_covers_all_booleans— future-proofing test that maintains two explicit sets (_PROPAGATEDand_INTENTIONALLY_FALSE) covering everyModelCardboolean field. If a new boolean is added toModelCard, this test fails and forces an explicit composite aggregation decision.Checklist
Additional Notes
The composite's
forward()dispatches directly to child models, so the missing fields did not break the forward path itself. The bug affected any generic code that inspects the composite'smodel_cardor callsinput_data()to determine required batch fields.Mock helpers in tests use
model_copy(update={...})instead of manually rebuilding partialModelCardinstances, avoiding accidental field omissions in test setup.