search

huggingface / transformers

🤗 Transformers: State-of-the-art Machine Learning for Pytorch, TensorFlow, and JAX.
https://huggingface.co/transformers
Apache License 2.0
133.25k stars 26.61k forks source link

length_penalty behavior is inconsistent with documentation #18208

Closed artidoro closed 2 years ago

artidoro commented 2 years ago

System Info

Who can help?

@patrickvonplaten

Information

Tasks

Reproduction

length_penalty in language generation has different effects on the the length of the generation. Sometimes it makes the generation longer, sometimes it makes it shorter. This is very confusing as it is different from what the documentation says. Two previous issues touch on this problem: #4915 #16930

In Bart CNN/DM length_penalty lengthens the output.

from transformers import pipeline
summarizer = pipeline("summarization", model='facebook/bart-large-cnn')
ARTICLE = """ New York (CNN)When Liana Barrientos was 23 years old, she got married in Westchester County, New York.
A year later, she got married again in Westchester County, but to a different man and without divorcing her first husband.
Only 18 days after that marriage, she got hitched yet again. Then, Barrientos declared "I do" five more times, sometimes only within two weeks of each other.
In 2010, she married once more, this time in the Bronx. In an application for a marriage license, she stated it was her "first and only" marriage.
Barrientos, now 39, is facing two criminal counts of "offering a false instrument for filing in the first degree," referring to her false statements on the
2010 marriage license application, according to court documents.
Prosecutors said the marriages were part of an immigration scam.
On Friday, she pleaded not guilty at State Supreme Court in the Bronx, according to her attorney, Christopher Wright, who declined to comment further.
After leaving court, Barrientos was arrested and charged with theft of service and criminal trespass for allegedly sneaking into the New York subway through an emergency exit, said Detective
Annette Markowski, a police spokeswoman. In total, Barrientos has been married 10 times, with nine of her marriages occurring between 1999 and 2002.
All occurred either in Westchester County, Long Island, New Jersey or the Bronx. She is believed to still be married to four men, and at one time, she was married to eight men at once, prosecutors say.
Prosecutors said the immigration scam involved some of her husbands, who filed for permanent residence status shortly after the marriages.
Any divorces happened only after such filings were approved. It was unclear whether any of the men will be prosecuted.
The case was referred to the Bronx District Attorney\'s Office by Immigration and Customs Enforcement and the Department of Homeland Security\'s
Investigation Division. Seven of the men are from so-called "red-flagged" countries, including Egypt, Turkey, Georgia, Pakistan and Mali.
Her eighth husband, Rashid Rajput, was deported in 2006 to his native Pakistan after an investigation by the Joint Terrorism Task Force.
If convicted, Barrientos faces up to four years in prison.  Her next court appearance is scheduled for May 18.
"""
print(summarizer(ARTICLE, max_length=512, min_length=30, do_sample=False, length_penalty=1))
print(summarizer(ARTICLE, max_length=512, min_length=30, do_sample=False, length_penalty=2))

Output: [{'summary_text': 'Liana Barrientos has been married 10 times, with nine of her marriages occurring between 1999 and 2002. She is believed to still be married to four men, and at one time, she was married to eight men at once.'}]

[{'summary_text': 'Liana Barrientos, 39, is charged with two counts of "offering a false instrument for filing in the first degree" In total, she has been married 10 times, with nine of her marriages occurring between 1999 and 2002. She is believed to still be married to four men.'}]

In GPT-2 increasing length_penalty shortens the output.

from transformers import pipeline
generator = pipeline('text-generation', model='gpt2', device=5)
print(generator("The White man worked as a", max_length=512, length_penalty=1))
print(generator("The White man worked as a", max_length=512, length_penalty=2))

Output: [{'generated_text': 'The White man worked as a receptionist for the British Consulate in Cairo and returned to Alexandria, where he was promoted to a military officer in 1953; in 1960 he worked as a consular officer, serving as secretary of state to President John F. Kennedy, and as a consul. In a conversation last fall, his grandfather told his sister Catherine, "We are going to make sure you are well."\n\nThe family is now living in a modest apartment, in a small part of town in the suburb of Alexandria.\n\n"We love you, and we love you," Catherine said, before she walked the five miles to the airport, where her husband, the first Egyptian president, has a $1 million plane ticket. The couple are still in touch with their three children, and will visit one next week.\n\nIn addition to the family, there are three other family members, one of whom has spent years as a caretaker for the hospital, which was the site of the largest civil conflict ever seen in modern Egypt. One was a nurse and family friend, who was paralyzed in a July 1975 accident.\n\n"It\'s just unbelievable," he told a reporter.\n\nThe funeral for one of the women who took her life last summer was held Wednesday at a church in the town of Dikun.\n\nIn his own words, the young woman\'s death marks a departure from his life.\n\n"I don\'t know if people would say I\'m the most important person in the world: I\'m the most beautiful person," he said. "But I did, but I will never forget that."'}]

[{'generated_text': "The White man worked as a mechanic.\n\nHe is said to have been very close with the White man's wife and three children. Other information came through during the early years of the investigation.\n\nPolice said they had asked the man to tell his story to police in order to gain information related to the white man's death.\n\nA source close to the father said the motive for the killings is still being investigated and the suspect was not a white man."}]

Expected behavior

Effect of length_penalty to be consistent with documentation.

Currently the documentation says: "Exponential penalty to the length. 1.0 means that the beam score is penalized by the sequence length. 0.0 means no penalty. Set to values < 0.0 in order to encourage the model to generate longer sequences, to a value > 0.0 in order to encourage the model to produce shorter sequences."

LysandreJik commented 2 years ago

cc @gante as well

gante commented 2 years ago

Hi @artidoro 👋 Thank you for raising this issue!

There are actually two distinct problems, the first one was already on my radar:

  1. length_penalty is only used with beam_search-based generation techniques. facebook/bart-large-cnn uses them by default, and gpt2 doesn't. So, in fact, length_penalty has no effect on gpt2, the different results you're seeing are a consequence of sampling being on by default for gpt2 (all these hidden defaults are also going through a deprecation phase 😉 ) 👉 solution: raise warnings/exceptions when these options have no effect (already being worked on)
  2. The docstring really describes the opposite of what happens. As described in #4915: larger length_penalty -> larger denominator, increasing with output length -> larger score (because it is a negative value), increasing with output length -> benefits long outputs 👉 solution: fix the docstring (@patrickvonplaten FYI)

I'll keep this issue open until the 2nd problem gets fixed.

patrickvonplaten commented 2 years ago

Confirming point 2.) @gante we could directly fix this here: https://github.com/huggingface/transformers/blob/06d1ba1a55a12b3fb3ca081bdd4f812fda800c37/src/transformers/generation_beam_search.py#L140 as well.