Fix the Samsung MobileBert test setup so it can run - #22550
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22550
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 1 PendingAs of commit 186f38a with merge base c65b23a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
The Samsung model test job runs every test under backends/samsung/test/models. One of those tests, the MobileBert fine-tuning test, always errors out before it does any work: ERROR: setUpClass (test_mobilebert_finetuning.Test_Milestone_MobileBertFinetune) AttributeError: type object 'Test_Milestone_MobileBertFinetune' has no attribute 'model_cache_dir' That single error fails the whole job, which in turn holds back the branch of known-good commits, so the other ten Samsung model tests passing does not help. There were three separate problems stacked in the test setup, and each one was hidden behind the one before it: 1. setUpClass read cls.model_cache_dir, which is not defined anywhere. 2. It passed that value to patch_mobilebert_finetuning(), which takes no arguments. Python only checks this when the call happens, so it could not be seen until the first problem was gone. 3. The setup replaced load_tokenizer with a copy of itself that left out the model name, so AutoTokenizer.from_pretrained() had nothing to load. The replacement tokenizer loader was the same as the real one except for the missing model name, so it could only ever do less. Removing the setup that installed it fixes all three problems at once and lets the test use the real loader. The test now looks like the other ten tests in the same directory, which build a model and check it with no extra setup. Test plan: Ran the test collection and setup for the file before and after the change with the heavy dependencies stubbed out. Before, it reproduces the same AttributeError seen in CI and never reaches the test body. After, setup succeeds and the test body is reached. Checked that model_cache_dir, setUpClass and the unused AutoTokenizer import are all gone, and that the file still compiles.
8b3027f to
186f38a
Compare
|
@claude review |
|
Claude finished @kirklandsign's task in 48s —— View job Code Review — PR #22550
Verdict: LGTM. The change is correct, well-scoped, and the reasoning in the PR description holds up against the source. What I checked
Minor observations (non-blocking, no change requested)
Nothing to fix. Clean deletion that unblocks the whole Samsung model test job. |
|
@kirklandsign stamp? |
Summary
The Samsung model test job runs every test under
backends/samsung/test/models. One of those tests, the MobileBert fine-tuning test, always errors out before it does any work:One error fails the whole job, so the other ten Samsung model tests passing does not help. The job only runs the tests when a Samsung device is available, which is why this looks like it comes and goes.
There were three separate problems stacked in the test setup, and each one was hidden behind the one before it:
setUpClassreadcls.model_cache_dir, which is not defined anywhere.patch_mobilebert_finetuning(), which takes no arguments. Python only checks the number of arguments when the call happens, so this could not be seen until the first problem was gone.load_tokenizerwith a copy of itself that left out the model name, soAutoTokenizer.from_pretrained()had nothing to load.The replacement tokenizer loader was the same as the real one except for the missing model name, so it could only ever do less than the real one. Removing the setup that installed it fixes all three problems at once and lets the test use the real loader. The test now has the same shape as the other ten tests in the same directory, which build a model and check it with no extra setup.
Test plan
Ran the real test collection and setup for this file before and after the change, with the heavy third party dependencies stubbed out so only the setup path was under test.
AttributeErrorseen in CI, and never reaches the test body.Also checked that
model_cache_dir,setUpClassand the now unusedAutoTokenizerimport are all gone, and that the file still compiles.