Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate: low memory tests are flaky #29136

Closed
wants to merge 1 commit into from
Closed

Generate: low memory tests are flaky #29136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tests/generation/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,7 @@ def test_contrastive_generate_dict_outputs_use_cache(self):
for output in (output_contrastive, output_generate):
self._check_outputs(output, input_ids, model.config, use_cache=True)

@is_flaky(description="low_memory changes the order of FP operations, which may result in different tokens.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think @flaky is appropriate here. This flag is for marking tests which occasionally fail due to randomness. Here it will fail for some models because of the change in operations, but that change is deterministic i.e. this test will still fail. Instead. this should be explicitly skipped for those models it affects.

Copy link
Member Author

@gante gante Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyeroberts It is a deterministic property, yes, but one that we can't control unless we control the inputs as well. Similar to how time measurement tests or equivalence tests are flaky (because the environment is not fully isolated and random inputs that may trigger an error above the threshold, respectivelly).

This means that we either control the inputs or add this flag. I have a strong preference for this flag -- otherwise, we may have to manually find a set of inputs that work for ALL models every time we make a modeling change in any language model (this is a mixin test) 👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that if it's deterministic, this flag won't fix things - it only triggers a re-run n times. Even if there's randomness in the inputs, the difference in the order of operations won't be affected i.e. I'd expect this to be very flaky for some models, and not for others. It doesn't make sense to say it "passes" for certain models because the model doesn't actually have this property.

Copy link
Member Author

@gante gante Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyeroberts Let me explain the whole problem better, so we can decide together on how to proceed on this class of tests as a whole 😄 (And sorry for the wall of text. This may be useful for future reference!)

The point of this test is to confirm that passing low_memory flag, which causes beam search to separate a batch into several mini-batches at model inference time so as to save memory, results in roughly the same outputs. The cause of the numerical differences is a matmul-level property we can't control: batched matmul vs several vector-matrix multiplications will have slightly different outputs, due to different shapes. Exactly the same way that passing a past_key_values cache results in slightly different outputs, the source and magnitude of numerical mismatches are the same. Ditto for assisted decoding vs non assisted decoding and left-padding vs not padded (the root cause of the last one is slightly different, the attention masking). In all these cases, one of the following is happening:

  1. the equivalence of using the feature or not is untested (past_key_values); OR
  2. the feature has a test with this decorator (assisted decoding); OR
  3. the feature has a test with an internal loop doing something similar to this decorator (left-padding); OR
  4. the feature has a test and fails occasionally (low_memory).
    Ideally, they should have the same pattern!

In a perfect world, we'd isolate and test the numerical property: the logits in these cases should be very close to one another ✅ [although we don't have a test for these properties in isolation, and we should!]

The catch is that these options (low_memory, past_key_values, left-padding, assisted decoding) do other things in generate in addition to triggering these numerical properties. So we should test that a) the resulting outputs are similar; AND b) the rest of the behavior associated with the option is correct (e.g. the reconstructed output tensors such as scores are correctly built). We also have the following influencing how we write these tests:

  1. Some models have modified a generate (e.g Whisper), and we want to check these options work well there;
  2. We can't simply compare that the logits are similar: due to the fluctuation in the logits, different tokens may be selected in a certain generate step, causing all subsequent logits to be vastly different;
  3. Comparing that the logits are similar up to the token differences is also not an option. It may hide a bug;
  4. The test should be a mixin, to facilitate the job of adding new models while ensuring correctness;
  5. Hardcoding an input in the mixin to cherry-pick a situation where these numerical differences do not materialize into different tokens is brittle. We risk having to find a new input every once in a while, as we add new models and make modeling corrections. Stochastic inputs may also enable us to uncover funny situations (a test that has a planned failure rate of 3% failing 5 times should be checked).

Having explained the problem, I believe that repeating the test N times is the only way to ensure the test goals are met, while respecting the influencing factors. It's not that the test is flaky... it's that we have to verify the results of a non-linear operation over an approximation with surrounding additional logic, which is hard.

I'm open to suggestions, as long as we don't limit our ability to detect problems -- if anything, our test suite is small 🤗

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a super interesting discussion: might be minor but indeed if @flaky is not the correct "explanation" - maybe a new decorator would be justified here, such as @nondeterministic, at least for readability/ulterior documentation purposes?
Second, for this kind of added-randomness tests, what do we think about using a non-exact match metric?

By that I mean, if the generation differs slightly, would something like a Levenshtein-based metric kept above some threshold be acceptable? Only for these tests, that is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be up for non-exact match metrics, but I'm not sure how to do it 🤔

These tests use our tiny random models, whose output is gibberish from a human perspective. Their output distribution tends to be flatter as well, which makes them more susceptible to the numerical fluctuations materializing into different tokens 😞 We can see in the tests that the divergence can happen as soon as in the first generation iteration. Ignoring the tokens, the best I could come up with was to compare the logits up to the divergence. Not even the shapes are required to match unless we remove the EOS token 😅

I'm strongly biased towards a solution that forces us to compare the whole output somehow, as I've been bitten by poor logic when iterating on these features. I couldn't detect the issue by comparing the model logits up to the divergence nor by checking the results qualitatively, only by comparing the full output against the unmodified case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, adding for traceability: it seems that on main, these tests are not flaky anymore? Or at least, not with a ~3% or 1% error rate - can't reproduce them on a loop on CPU or GPU machines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@molbap Is this for all models, or just the ones which were previously failing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyeroberts I think, all? Specifically, I ran

for i in {1..1000}; do
  RUN_SLOW=1 CUDA_VISIBLE_DEVICES= python -m pytest tests/ -k test_contrastive_generate_low_memory >> test_runs.log 2>&1
done

and looked at the logs, saw only green there!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh - OK. Well that's both good news and also perplexing. In this case, let's close this PR and if they crop up again we can figure how to handle (and what might be causing this intermittent behaviour)

def test_contrastive_generate_low_memory(self):
# Check that choosing 'low_memory' does not change the model output
for model_class in self.all_generative_model_classes:
Expand Down Expand Up @@ -1542,6 +1543,7 @@ def test_contrastive_generate_low_memory(self):
)
self.assertListEqual(low_output.tolist(), high_output.tolist())

@is_flaky(description="low_memory changes the order of FP operations, which may result in different tokens.")
def test_beam_search_low_memory(self):
# Check that choosing 'low_memory' does not change the model output
for model_class in self.all_generative_model_classes:
Expand Down
Loading