Refactor Rouge and Meteor to InstanceMetric for faster score computation

[yoavkatz]

added a new metric with interval calculations

[OfirArviv]

@yoavkatz maybe change it in the class itself? Not only in the catalog? That way if people fetch the metric as I did they still have the default

[yoavkatz]

@yoavkatz maybe change it in the class itself? Not only in the catalog? That way if people fetch the metric as I did they still have the default

Meteor (unlike rouge) does not have a class, and only is an instantiation of Huggingface metric.

I think the Rouge reason it is slow , is that in itself it does bootstrapping in the code:

https://huggingface.co/spaces/evaluate-metric/rouge/blob/main/rouge.py#L134

I think the solution is to avoid use aggregation.

[dafnapension]

@eladven , @yoavkatz , I think that the problem with the use of unitxt of HF Rouge and Meteor (and others) is in making them Global, and even leaving the default arg of GlobalMetric: process_single_instances = True in place.
In such a case, HF, that by itself invokes heavy language models, is invoked first for each and every instance (from the beginning of GlovalMetric.process()), and then for the whole stream (from the second part of that process()). When HF is invoked for the whole stream, it again processes each and every instance.
Now if this happens with CI - the processing of each and every instance (which is heavy language model) repeats 2 * number of resamples.
If these HF metric are incarnated as InstanceMetric (and there is no reason why not), then each instance would have been processed just once! the CI uses existing scores, if there are, it does not compute them again, and there will be if so incarnated as InstanceMetric.

I think it is better to fix that issue and not flood the catalog with cards of yes/no CI.

[dafnapension]

Please see my comment before you merge. I think it is not a good idea to run away from CI when this is not necessary.

[yoavkatz]

@eladven , @yoavkatz , I think that the problem with the use of unitxt of HF Rouge and Meteor (and others) is in making them Global, and even leaving the default arg of GlobalMetric: process_single_instances = True in place. In such a case, HF, that by itself invokes heavy language models, is invoked first for each and every instance (from the beginning of GlovalMetric.process()), and then for the whole stream (from the second part of that process()). When HF is invoked for the whole stream, it again processes each and every instance. Now if this happens with CI - the processing of each and every instance (which is heavy language model) repeats 2 * number of resamples. If these HF metric are incarnated as InstanceMetric (and there is no reason why not), then each instance would have been processed just once! the CI uses existing scores, if there are, it does not compute them again, and there will be if so incarnated as InstanceMetric.

I think it is better to fix that issue and not flood the catalog with cards of yes/no CI.

I agree. I changed the code of Rouge to be HuggingfaceBulkMetric and it solved the issue for Rouge.

Please review and let me know what you think.

Regarding Meteor - can you do the appropriate thing?

[dafnapension]

yes, coming up

[dafnapension]

Hi @yoavkatz , I added Meteor as an HuggingFaceInstance, to see how this works. With ci.
Also, I thought about rouge: if it is bulk, then each bulk gets the global score as the average of the bulk, not the average of the whole stream. Is that OK?
So I changed Rouge to inherit from this new HuggingFaceInstance. Nothing else changed.
I also fixed a small bug in test_metric_utils, and things seem to be fine now, here, on my little laptop.
I hope that this new incarnation will survive also out in the wild fields of fm_eval..

[yoavkatz]

Hi @yoavkatz , I added Meteor as an HuggingFaceInstance, to see how this works. With ci. Also, I thought about rouge: if it is bulk, then each bulk gets the global score as the average of the bulk, not the average of the whole stream. Is that OK? So I changed Rouge to inherit from this new HuggingFaceInstance. Nothing else changed. I also fixed a small bug in test_metric_utils, and things seem to be fine now, here, on my little laptop. I hope that this new incarnation will survive also out in the wild fields of fm_eval..

In general BulkInstanceMetric does not perform aggregation, and assume the results returned per instance. It just a matter of optimization, for example in LLM based metrics we send a batch of requests at a time , instead of 1-1 which is much slower.

In Rouge, I removed the use_aggregation=True to the metric returns for each instance the rouge score, which is then averaged by the generic BulkInstanceMetric code.

[yoavkatz]

@dafnapension - it would be if we add a unit test that checks that if we wrap a metric (like Rouge) in HuggingfaceInstanceMetric or HuggingfaceBulkInstanceMetric - we would get the same results.

[dafnapension]

A good idea, coming up!

[dafnapension]

Added a comparison of Meteor to its previous global HF implementation, upon preparation of the metric (in Meteor.py).
Now looking into Rouge.

[dafnapension]

Hi @yoavkatz , I added a test comparing old and new rouge - to show they return the same result.
Also, per a good advice from @arielge and @assaftibm , in case of more than 100 instances, I changed the method of bootstrap from "BCa" (the default) to "percentile".

Last: please note that for rouge, if HF's use_aggregator = False , then HF returns vectors of numeric results (rather than simple floats), with which we can not do CI. So I made the comparison above with use_aggregator = True in order to allow CI. The bootstrapping that HF does per its own initiative (when use_aggregator = True) does not change much the result, compared to a simple average over the individual instance scores.

[yoavkatz]

What the run time difference and accuracy between BCa and percentile?

[dafnapension]

I do not really know.. I took the advice of @arielge and @assaftibm , here:
#1008 (comment)
and
#1008 (comment)

[dafnapension]

I found information about percentile and BCa here.
From my understanding (@assaftibm , @arielge , please correct my mistakes..),
'percentile' works by the following intuitive notion: make n_resamples of the population [the stream fed to metric], each such resample is of the size of the population and is built by repeating the following process: independently, and identically distributed, select an item [instance] from the population, with replacement [return of a selected item back to the population].
Then, given one resample, compute the statistics (the metric) on it, the same way the metric is computed on the original population.
Then, given n_resamples scores (one per re_sample), return the 0.05 and 0.95 percentile ("quintile" of 100 rather than 5) as the ci borders.

BCa continues the above result of n_resamples scores, by correcting skewness and biases thereof, observed when comparing to the original population.

[yoavkatz]

I'm not sure it matters, but I saw somewhere the BCa is the one that is commonly used. If the difference in runtime is small (I would think the main cost is recalculating the metric and not the BCa vs percentile), then I would keep it simple and not change behavior on some threshold.

So - can you try running twice (with BCA and with percentile) on say 200 instances, and see the difference in runtime?

[assaftibm]

Hi @dafnapension!

Yes - I think your understanding of Percentile bootstrap is correct.

I would suggest the following:

1. When it's an InstanceMetric, use BCa bootstrap.
2. When it's a GlobalMetric, use Basic bootstrap (or Percentile, see discussion below).

In 2, I wouldn't condition the type of bootstrap on the size of the dataset and switch between BCa and other lighter bootstrap because it can confuse people that test a full dataset vs. smaller samples of it - they will suddenly get different CIs due to a switch in the bootstrap type. I think it's better to stick to one for consistency.

Regarding Basic vs. Percentile bootstrap.
There is a description here. I have to admit that I don't fully understand the computation of the Basic bootstrap. However, note that Scipy documentation includes this comment:

While the 'percentile' method is the most intuitive, it is rarely used in practice. Two more common methods are available, 'basic' (‘reverse percentile’) and 'BCa' (‘bias-corrected and accelerated’); they differ in how step 3 is performed.

So this is the main reason why I would lean towards Basic over Percentile. It would be easier if we could stick to BCa also in GlobalMetrics but unfortunately that doesn't seem practical.

As a side note, I think it makes sense to ask about the slowdown with BCa on StackOverflow - showing a short self-contained sample code that reproduces the problem using Scipy's implementation and asking if this is as expected. It's possible that the implementation in Scipy is not optimal.

[dafnapension]

In all my experiments, because of lack of real data, I used prediction=target (first reference). I think it only changes a constant time of computing each instance score, so I allowed myself..
In my humble opinion, in the numbers of instances and resamples that I see in the cards, we can stick to the said to be most commonly used BCa. Just remember to check again if we grow dramatically.

[yoavkatz]

Thanks for the detailed analysis. BCa seems to increase the time about 10%-20% in instance metrics, but more than doubles the time in global metrics.

[yoavkatz]

So do we agree to keep BCa always? I don't think we have many more global metrics that take a long time - and it's the simplest approach.

[dafnapension]

I, too, think so. Leave the change for a future consideration, if and when needed.
@elronbandel also asked that I simply copy over the code from what we find in HF to an in-house instance metric, for a further speedup.

[assaftibm]

I think the problem will appear with more complex global metrics like CorpusBLEU, but I agree it can be dealt with in a separate PR.

[dafnapension]

Hi @yoavkatz , looking at these lines in HuggingfaceBulkMetric:

unitxt/src/unitxt/metrics.py

Line 1305 in 2381c4e

results = [{} for _ in range(len(scores[self.hf_metric_fields[0]]))]

it seems like you assume that every score that is a list, means one component per instance. It is true for Rouge, but not so in bleu, for example, where a score named precisions is an array whose length equals the input argument max_order.
I think that this breakdown of list scores to instance score should be done in the individual metric level, and not in the BulkMetric level.

[yoavkatz]

Hi @yoavkatz , looking at these lines in HuggingfaceBulkMetric:

unitxt/src/unitxt/metrics.py

Line 1305 in 2381c4e

results = [{} for _ in range(len(scores[self.hf_metric_fields[0]]))]

it seems like you assume that every score that is a list, means one component per instance. It is true for Rouge, but not so in bleu, for example, where a score named precisions is an array whose length equals the input argument max_order.
I think that this breakdown of list scores to instance score should be done in the individual metric level, and not in the BulkMetric level.

I suggest we complete this PR on Rouge and Metric, and then think about Bleu and generalizing this.

[yoavkatz]

Note that there is HuggingfaceInstanceMetric which is used by Rouge and Meteor and HuggingfaceBulkMetric, which is only used by BertScore as far as I understand.

[dafnapension]

Hi @yoavkatz , indeed I was referring to a generalization (for a future PR), trying to address your comment that performance-wise, if possible, better approach HF in batches.

[elronbandel]

Suggested change

	nltk.download("wordnet")
	nltk.download("omw-1.4")
	nltk.download("wordnet", quiet=True)
	nltk.download("omw-1.4", quiet=True)

[elronbandel]

move to prepare so import is performed only once

[dafnapension]

thanks, @elronbandel , at last I learned (from you, as always) how to avoid those "already up to date" running on my screen.

[elronbandel]

@dafnapension looks great. only need to move imports to prepare() and make nltk downloads quiet ( see my comments)

Also consider making new PR only with your changes since there is this .secrets.basline that seem to be affected by wrong merging of the main

[elronbandel]

move to prepare

[elronbandel]

You need to save it under self.scorer then use it in the compute

[dafnapension]

Hi @elronbandel , Done.

[yoavkatz]

@elronbandel @dafnapension - can we merge these?

Updated