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.

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

Error in chriskonnertz/deeply/src/ChrisKonnertz/DeepLy/ResponseBag/TranslationBag.php #2

Closed
domih opened this issue Sep 18, 2017 · 18 comments
Assignees
Labels

Comments

@domih
Copy link

domih commented Sep 18, 2017

Note: not urgent, just trying it out.

$text = <<<EOS

The Center for American Progress puts it simply: “Americans work longer hours than workers in most other developed countries, including Japan, where there is a word, ‘karoshi’, for ‘death by overwork.’ The typical American middle-income family puts in an average of 11 more hours a week in 2006 than it did in 1979.” The end result is that we all live in a structurally toxic work world to use the title of an article written by Anne-Marie Slaughter, the author of “Unfinished Business: Women Men Work Family” published in 2015. The situation is bad for men and even worse for women. So what can we as individuals do about it without jeopardizing our job and reputation? Scott McDowell offers a useful starting point in a FastCompany article: Steps We Can All Take To Defy Our Culture Of Overwork:

If you are a boss, reduce your team’s hours.
Take your vacations.
Schedule free time.
Let your mind wander.
Stop talking how busy you are.

“In the past labor unions fought for worker sanity,” Scott McDowell adds. “In our current work culture we have to take individual responsibility for our well-being and for that of those around us. It’s time to alter the norms from within; our economy, health, productivity, and happiness depend on it.”
EOS;

$translatedText = $deepLy->translate($text, DeepLy::LANG_EN, DeepLy::LANG_FR);

Returns:

( ! ) Notice: Undefined offset: 0 in /var/www/html/libs/vendor/chriskonnertz/deeply/src/ChrisKonnertz/DeepLy/ResponseBag/TranslationBag.php on line 81
Call Stack

Time Memory Function Location

1 0.0006 367048 {main}( ) .../tryme.php:0
2 0.0103 1837440 ChrisKonnertz\DeepLy\DeepLy->translate( ) .../tryme.php:35
3 3.2932 1864856 ChrisKonnertz\DeepLy\ResponseBag\TranslationBag->getTranslation( ) .../DeepLy.php:239

( ! ) Notice: Trying to get property of non-object in /var/www/html/libs/vendor/chriskonnertz/deeply/src/ChrisKonnertz/DeepLy/ResponseBag/TranslationBag.php on line 87
Call Stack

Time Memory Function Location

1 0.0006 367048 {main}( ) .../tryme.php:0
2 0.0103 1837440 ChrisKonnertz\DeepLy\DeepLy->translate( ) .../tryme.php:35
3 3.2932 1864856 ChrisKonnertz\DeepLy\ResponseBag\TranslationBag->getTranslation( ) .../DeepLy.php:239
Le Center for American Progress le dit simplement:"Les Américains travaillent plus d'heures que les travailleurs dans la plupart des autres pays développés, y compris au Japon, où il y a un mot," karoshi ", pour" mort par surmenage "; la famille américaine typique à revenu moyen consacre en moyenne 11 heures de plus par semaine en 2006 qu'en 1979;" Le résultat final est que nous vivons tous dans un monde du travail structurellement toxique pour utiliser le titre d'un article écrit par Anne-Marie Sla ". La situation est mauvaise pour les hommes et pire encore pour les femmes. Que pouvons-nous donc faire, en tant qu'individus, pour y remédier sans mettre en péril notre travail et notre réputation? Prenez vos vacances. Prévoir du temps libre. Laisse ton esprit vagabonder. Arrête de dire à quel point tu es occupé. Dans le passé, les syndicats se sont battus pour la santé mentale des travailleurs ", ajoute Scott McDowell. Dans notre culture de travail actuelle, nous devons assumer la responsabilité individuelle de notre bien-être et de celui de ceux qui nous entourent. Il est temps de changer les normes de l'intérieur; notre économie, notre santé, notre productivité et notre bonheur en dépendent."

@chriskonnertz chriskonnertz self-assigned this Sep 18, 2017
@chriskonnertz
Copy link
Owner

Hello,

thank you for creating this issue. I will take a look at this!

@chriskonnertz
Copy link
Owner

chriskonnertz commented Sep 18, 2017

Which version did you use? Please take a look at this constant: ChrisKonnertz\DeepLy\DeepLy::version

On the master branch it works fine:

<?php
use ChrisKonnertz\DeepLy\DeepLy;

/**
 * Minimal class autoloader
 *
 * @param string $class Full qualified name of the class
 */
function miniAutoloader($class)
{
    require __DIR__ . '/../src/' . $class . '.php';
}

spl_autoload_register('miniAutoloader');

$deepLy = new ChrisKonnertz\DeepLy\DeepLy();

$text = <<<EOS
The Center for American Progress puts it simply: “Americans work longer hours than workers in most other developed countries, including Japan, where there is a word, ‘karoshi’, for ‘death by overwork.’ The typical American middle-income family puts in an average of 11 more hours a week in 2006 than it did in 1979.” The end result is that we all live in a structurally toxic work world to use the title of an article written by Anne-Marie Slaughter, the author of “Unfinished Business: Women Men Work Family” published in 2015. The situation is bad for men and even worse for women. So what can we as individuals do about it without jeopardizing our job and reputation? Scott McDowell offers a useful starting point in a FastCompany article: Steps We Can All Take To Defy Our Culture Of Overwork:

    If you are a boss, reduce your team’s hours.
    Take your vacations.
    Schedule free time.
    Let your mind wander.
    Stop talking how busy you are.

“In the past labor unions fought for worker sanity,” Scott McDowell adds. “In our current work culture we have to take individual responsibility for our well-being and for that of those around us. It’s time to alter the norms from within; our economy, health, productivity, and happiness depend on it.”
EOS;

$translatedText = $deepLy->translate($text, DeepLy::LANG_EN, DeepLy::LANG_FR);

echo '<pre>'.$translatedText.'</pre>';

Printed:

Le Center for American Progress le dit simplement:"Les Américains travaillent plus d'heures que les travailleurs dans la plupart des autres pays développés, y compris au Japon, où il y a un mot," karoshi ", pour" mort par surmenage "; la famille américaine typique à revenu moyen consacre en moyenne 11 heures de plus par semaine en 2006 qu'en 1979;" Le résultat final est que nous vivons tous dans un monde du travail structurellement toxique pour utiliser le titre d'un article écrit par Anne-Marie Sla ". La situation est mauvaise pour les hommes et pire encore pour les femmes. Que pouvons-nous donc faire, en tant qu'individus, pour y remédier sans mettre en péril notre travail et notre réputation? Scott McDowell offre un point de départ utile dans un article de FastCompany: Steps We Can All Take To Defy Our Culture Of Overwork:" La culture du surmenage: Si vous êtes un patron, réduisez les heures de travail de votre équipe. Prenez vos vacances. Prévoir du temps libre. Laisse ton esprit vagabonder. Arrête de dire à quel point tu es occupé. Dans le passé, les syndicats se sont battus pour la santé mentale des travailleurs ", ajoute Scott McDowell. Dans notre culture de travail actuelle, nous devons assumer la responsabilité individuelle de notre bien-être et de celui de ceux qui nous entourent. Il est temps de changer les normes de l'intérieur; notre économie, notre santé, notre productivité et notre bonheur en dépendent."

(PHP 7.0.13, Apache, Windows, error_reporting E_ALL)

@chriskonnertz
Copy link
Owner

chriskonnertz commented Sep 18, 2017

However, I have added some lines of code that should catch the problem before a notice is created and throw a nice exception.

9ad9a48

@domih
Copy link
Author

domih commented Sep 18, 2017

const VERSION = '1.3.0';

...$ composer require chriskonnertz/deeply
Using version ^1.3 for chriskonnertz/deeply
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 1 install, 0 updates, 0 removals

  • Installing chriskonnertz/deeply (v1.3.0): Downloading (100%)

@domih
Copy link
Author

domih commented Sep 18, 2017

By the way, I'm using PHP 7.1.9.

@chriskonnertz
Copy link
Owner

Strange. The Travis-CI tests cover PHP 7.1: https://github.com/chriskonnertz/DeepLy/blob/master/.travis.yml#L6

@chriskonnertz
Copy link
Owner

Can you please update again (to version 1.3.1), then run the code and tell me if you still get the same exception or if it is a BagException?

@domih
Copy link
Author

domih commented Sep 19, 2017

1.3.1 outputs:

DeepLy API call resulted in a malformed result - beams array is empty in translation 3

@domih
Copy link
Author

domih commented Sep 19, 2017

Note: the translation goes through OK when directly using https://www.deepl.com/translator

@domih
Copy link
Author

domih commented Sep 19, 2017

I added some logging to CurlHttpClient::callApi(...):

public function callApi($url, array $payload, $method)
{
	
    \Util::logDebug("CurlHttpClient::callApi($url, ..., $method)");
			
    //...
			
    \Util::logDump("Deeply / \$jsonData\n\n", $jsonData);
    \Util::logDump("Deeply / \$jsonData pretty\n\n", \Util::jsonPrettyPrint($jsonData));
    \Util::logDump("Deeply/ \$jsonData as array\n\n", json_decode($jsonData, true));
    
    $rawResponseData = curl_exec($curl);
    
    \Util::logDump("Deeply / \$rawResponseData\n\n", $rawResponseData);
    \Util::logDump("Deeply / \$rawResponseData pretty\n\n", \Util::jsonPrettyPrint($rawResponseData));
    \Util::logDump("Deeply / \$rawResponseData as array\n\n", json_decode($rawResponseData, true));
			
    //...
			
}

See attached resulting log.
wr-2017-09-19-18.log.txt

@domih
Copy link
Author

domih commented Sep 19, 2017

If, you comment out this:

// if (sizeof($translation->beams) == 0) {
// throw new BagException(
// 'DeepLy API call resulted in a malformed result - beams array is empty in translation '.$index
// );
// }

And add further down the test on the array size:

    foreach ($this->responseContent->translations as $translation) {
        // The beams array contains 1-n translation alternatives.
        // The first one (index 0) is the "best" one (best score)
    	if(count($translation->beams)){
        $beam = $translation->beams[0];

        if ($translatedText !== '') {
            $translatedText .= ' ';
        }

        $translatedText .= $beam->postprocessed_sentence;
    	}
    }

It goes through. So the question you should ask yourself: is an empty beams array an error?

@domih
Copy link
Author

domih commented Sep 19, 2017

Finally, I have these questions:

  1. Why do you need to call the API twice?
  2. When using https://www.deepl.com/translator the carriage returns are maintained. When using Deeply, there are eliminated. Is there a way to maintain them?
    TIA!

@chriskonnertz
Copy link
Owner

It goes through. So the question you should ask yourself: is an empty beams array an error?

"It goes through" = So you get the translation then?

"is an empty beams array an error" I don't know, welcome to the "let's guess how the API works" game. ;-) I guess it is an error but this is just a guess. My big problem is that I am not able to reproduce the behaviour that you have experienced. So all I do is guessing / assuming what might cause the issue.

@chriskonnertz
Copy link
Owner

chriskonnertz commented Sep 20, 2017

Why do you need to call the API twice?

The reason is a limitation of the API. It is not able to translate a text (=one string) that consists of multiple sentences. However it can translate multiple sentences (=array of strings) in one API call. And it is able to split a text into sentences. So the current implementation first makes a call that splits texts into sentences and then makes antoher call that translates these sentences.

Since the API is not documented I do not know if it is possible to merge both calls into one "split and translate" call - which would be great.

If you only want to translate a single sentence, you can call the translate($text, $to = self::LANG_EN, $from = self::LANG_AUTO, $joinSentences = false) method with joinSentences set to true.

If you have an array of sentences there will be a method that accepts an array as parameter and directly translates the sentences (=array items) without an additional API call. However, that has not been implemented yet.

@chriskonnertz
Copy link
Owner

chriskonnertz commented Sep 20, 2017

When using https://www.deepl.com/translator the carriage returns are maintained. When using Deeply, there are eliminated. Is there a way to maintain them

This happens when the text is split into sentences. DeepLy sends something like "Hello world!\r\nHow are you?" (string) to the API and the response is ["Hallo Welt!", "Wie geht es dir?"] (array)". So this happens outside of DeepLy. When joining the sentences to a single string DeepLy could at line break but between all sentences but that would be pure guessing. So I guess the best way would be to split the text via $parts = explode('\r\n', $text), then let the API split all $parts and translate them and then put them together via $text = implode(PHP_EOL, $translatedParts).

@chriskonnertz
Copy link
Owner

Made a commit: ada88e9

The translateSentences() method now expects an array of strings (sentences) as the first parameter and internally only makes one API call.

@domih
Copy link
Author

domih commented Sep 20, 2017

"It goes through" = So you get the translation then?

Yes.

@chriskonnertz
Copy link
Owner

chriskonnertz commented Sep 27, 2017

I'll close this issue. I cannot reproduce the error so I cannot really fix it. I made two commits that may help a little bit and I added the idea of keeping line breaks to the to-do-list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants