Skip to content

Fix Transformer ignoring a custom encoder or decoder with no parameters - #3962

Open
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:fix-transformer-custom-modules
Open

Fix Transformer ignoring a custom encoder or decoder with no parameters#3962
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:fix-transformer-custom-modules

Conversation

@ayaangazali

Copy link
Copy Markdown
Contributor

Proposed changes

Transformer picks its submodules with custom_encoder or TransformerEncoder(...). Since Module subclasses dict, a module that holds no arrays is an empty dict and therefore falsy, so a parameterless custom encoder or decoder gets silently thrown away and the default one is built instead:

import mlx.core as mx
import mlx.nn as nn

bool(nn.Identity())   # False

t = nn.Transformer(dims=8, num_heads=2, num_encoder_layers=3, num_decoder_layers=1,
                   custom_encoder=nn.Identity())
type(t.encoder)       # TransformerEncoder, not Identity

Same for custom_decoder. It only shows up when the custom module has no parameters of its own, so a custom encoder that happens to hold a weight works fine and this looks inconsistent rather than broken. Switched both to an explicit is not None check.

Added a small test for it. It fails on main with the current truthiness check and passes with this change.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

(test_nn.py passes; happy to drop the test if you would rather keep this to the one line change)


context on me: freshman contributor, and i lean on Claude Code when hunting for things like this, but i confirmed the empty-module-is-falsy behaviour myself and checked that a custom module holding a parameter behaves differently, which is what made it click. if you would rather express the fix as a conditional expression than an if/else, say the word.

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.

1 participant