Deprecate mae pixio decoder timm - #2006
Conversation
4611b13 to
f37c5c6
Compare
|
@gabrielfruet this is the follow-up PR to deprecate MAEDecoderTIMM and PixioDecoderTIMM. |
gabrielfruet
left a comment
gabrielfruet
left a comment
There was a problem hiding this comment.
The migration is correct. I ran the MAE flow end to end and the mask semantics match the old code. Tests pass: 18 passed, 4 skipped.
Two blockers. PixioDecoderTIMM users never see the deprecation warning, and the benchmark lost the decoder weight init. The other comments are smaller.
One more that will not anchor, because lightly/models/modules/__init__.py is not in the diff.
lightly/models/modules/__init__.py, the MAEDecoderTIMM, PixioDecoderTIMM import
This warns nobody who imports the name and never makes an instance. After the removal, those users get an ImportError with no hint.
A module __getattr__ covers both cases. It needs Python 3.7. Our CI floor is 3.7.
def __getattr__(name: str) -> Any:
if name in ("MAEDecoderTIMM", "PixioDecoderTIMM"):
from lightly.models.modules import masked_autoencoder_timm
warn_deprecated(name, "MaskedVisionTransformerDecoderTIMM", removed_in="1.7.0")
return getattr(masked_autoencoder_timm, name)
raise AttributeError(name)Remove the eager import. If the attribute already exists, __getattr__ never runs. Keep the timm_vit_available() guard.
Keep the warning in the constructor too. A direct import from lightly.models.modules.masked_autoencoder_timm does not pass through here.
Filed #2019 for the repo-wide part, which is out of scope here.
| self.sequence_length = vit.patch_embed.num_patches + 1 | ||
| self.backbone = MaskedVisionTransformerTIMM(vit=vit) | ||
| self.decoder = MAEDecoderTIMM( | ||
| self.decoder_embed = nn.Linear(vit.embed_dim, decoder_dim) |
There was a problem hiding this comment.
decoder_embed and prediction_head are plain nn.Linear layers outside the decoder. Nothing initializes them now. Before, _initialize_weights reached them through self.apply(init_weights): xavier uniform weights, zero bias.
At 768 to 512, the decoder_embed weight std moves from 0.0395 to 0.0208, and the max absolute bias moves from 0.0 to 0.0359. This changes what the benchmark trains.
self.decoder_embed.apply(init_weights)
self.prediction_head.apply(init_weights)test_matches_mae_decoder_full_flow cannot catch this. It copies the weights before it compares. examples/pytorch/mae.py and the other examples have the same gap. That matters more, because people copy the examples.
There was a problem hiding this comment.
Great catch! Restored, decoder_embed and prediction_head now get init_weights (xavier uniform, zero bias), matching the old decoder init. On the examples having the same gap, they are outside this PR's diff, so I would suggest a small follow-up (or folding it into #2019).
- add lightly/utils/deprecation.py::warn_deprecated: FutureWarning (shown by the default filter, unlike DeprecationWarning) with the 1.7.0 removal version, called from each decoder __init__ so the stacklevel points at the caller - add a package __getattr__ so importing the deprecated names warns and, once removed, raises AttributeError instead of a hintless ImportError - restore the imagenette benchmark decoder init (apply init_weights to decoder_embed and prediction_head) and take sequence_length from the backbone
|
@gabrielfruet Thanks for the review! Addressed most of it, left a few conversations open with some comments, let me know your thoughts how to proceed. |
gabrielfruet
left a comment
gabrielfruet
left a comment
There was a problem hiding this comment.
Everything from the last round looks right, thanks. One reversal from me below — the __getattr__ I suggested isn't worth it, sorry for the churn.
2 participants
Follow-up to #1978. Now that MaskedVisionTransformerDecoderTIMM covers both, this deprecates the old TIMM decoders, as discussed in the review (#1978 (comment)).