Skip to content

Add phi tests#1372

Merged
jlarson4 merged 3 commits into
TransformerLensOrg:devfrom
TensorCruncher:add-phi-tests
Jun 9, 2026
Merged

Add phi tests#1372
jlarson4 merged 3 commits into
TransformerLensOrg:devfrom
TensorCruncher:add-phi-tests

Conversation

@TensorCruncher

@TensorCruncher TensorCruncher commented Jun 9, 2026

Copy link
Copy Markdown

Description

Unit test file for Phi adapter.

In light of the new guidelines for unit tests, I have made the following inclusions. Let me know if they need to be removed / changed:

  1. Checking test_positional_embedding_type_is_rotary and test_parallel_attn_mlp_is_true. The prior is tested via component mapping and architecture guards, but since it is an override, I test it none the less. For the latter as well, it overrides a default, so it is tested despite having checks in component mapping (Parallel block bridge) and architecture guards (no ln2).

  2. test_use_fast_is_false checks for attribute (assert adapter.cfg.use_fast is False) instead of checking dictionary in class definition. Is this acceptable?

  3. Under component mapping, we check for explicitly set flags in the adapter: test_ln_final_use_native_layernorm_autograd_is_true,
    test_blocks_ln1_use_native_layernorm_autograd_is_true
    test_attn_requires_attention_mask_is_true
    test_attn_requires_position_embeddings_is_true

Related to #1302

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have not rewritten tests relating to key interfaces which would affect backward compatibility

@jlarson4

jlarson4 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@TensorCruncher Great work on this. The suite shape is right and the test coverage is good. Every category from the unit-test guide is represented, and the conversion/guard/setup-testing classes are exemplary. For the two flag tests you specifically flagged:

  1. test_positional_embedding_type_is_rotary and test_parallel_attn_mlp_is_true are redundant. Their effects are already covered by your component-mapping tests (test_rotary_emb_is_rotary_embedding_bridge + test_no_pos_embed_component, test_blocks_is_parallel_block_bridge + test_no_ln2_in_blocks). Please delete both.
  2. test_use_fast_is_false is fine to keep. It is a solid test and its effect isn't observable in the unit suite. Same for test_default_prepend_bos_is_false. The four explicit-kwarg tests under component mapping (use_native_layernorm_autograd, requires_attention_mask, requires_position_embeddings) are also fine.

@TensorCruncher

Copy link
Copy Markdown
Author

I have removed the two redundant config literal tests. Thank you for your review and kind words!

@jlarson4

jlarson4 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

You're welcome! Merging, it will be in the next release!

@jlarson4 jlarson4 merged commit 35ab438 into TransformerLensOrg:dev Jun 9, 2026
49 of 50 checks passed
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.

2 participants