Configuration emits wrong error messages

[tomasmcz]

See Travis build on branch err2. The error is on line 85 in the ini file, not 84. And it clearly isn't a syntax error, I just named a non-existent entity.

[tomasmcz]

Also, shouldn't syntax errors only occur during parsing and modules be imported after parsing? This should be impossible if the parsing and execution really are separated.

[jindrahelcl]

Fixed.

[tomasmcz]

So now nothing is a syntax error? I don't think that was the point here. And as always, commited and pushed directly to the master with no discussion. So should I close this and open feature request for errors to say "syntax error" when there is a syntax error?

Also, didn't Neural Monkey use to say something useful like "Cannot interpret [offending string here] as a module"? That would be much better.

Also, how is it possible that this was fixed entirely in a file called parsing.py, when @jlibovicky insists that parsing and execution are separated? Do you check whether the class exists without importing the module?

[jindrahelcl]

1. You complained about IniSyntaxError - this is what the class was called and from my perspective, you can hardly call it a bug. I concentrated mainly on the line numbers. If you please, open another issue. Or better, solve one of your own issues, e.g. beam search.
2. It did not say anything more useful than it says now.
3. Your insight is outdated. Anyway, where else would you expect a parsing error than in the parsing module. The module is imported allright. But it does not execute anything. The initialization of objects is done in config_loader

[tomasmcz]

1. So, you misunderstood the issue, wrote a "fix" that causes more inaccurate error messages that there was before and pushed it into master without discussion. The issue was that the example in my branch was reported as syntax error, although it isn't one. Now none of the syntax errors that were correctly reported before is reported as a syntax error. You could have deleted the whole module and claim that to be a fix – the error would also no longer exist.
2. It used to say something like "decoding_function.Attentioner cannot be interpreted as a module or whatever". The name was there. Now it says "Error". How is the first not infinitely more useful?
3. But this is not a parsing error, that's the point. Name of a non-existent class is syntactically correct and therefore has nothing to do with parsing. And if haven't noticed, code gets executed when you import a module in Python. We should parse the name, save it, parse the rest of the file, then execute the configuration. It might be a good idea to insert another check between the two, but that's a different issue.

[tomasmcz]

Also, what do you mean by "your insight is outdated"? The only explanation I can think of is that after @jlibovicky's refactor, which we discussed to great lengths and where we agreed that parsing should be split from execution, your subsequent refactor merged them back together. I hope I'm misinterpreting your remark.

[jindrahelcl]

radši se koukni na ten kód a když ti něco nebude vonět, tak to fixni sám

[jlibovicky]

It was me who did not realize that importing modules actually runs the code and the classes and functions were always imported during the parsing, it is a good point, indeed. Very strictly speaking, it is no a syntax error, but for instance Java compiler would call referring to a non-existing type a syntax error as well (although it does not run any code, of course).

It looks like it is possible to check whether a module exists without running its code (http://stackoverflow.com/questions/14050281/how-to-check-if-a-python-module-exists-without-importing-it), so ideally, I would check it while parsing and postpone the actual import to the execution phase.

[tomasmcz]

@jlibovicky: In case of Java compiler, there are "syntax" errors and runtime errors. Java can regard import error as "syntax" error, because it knows at compile time which names are defined. In Python, there also are also syntax errors (e.g. def f x ():), but import error is a runtime error, because you cannot know if something exists until you try to run it (and sometimes not even then). I'm not sure whether checking without importing is actually a good idea, because some modules (e.g. numpy) create their namespaces dynamically only when they are imported and we may want to use these in the configuration one day. Even though I'm all in favor of static checking of everything possible, this is Python, where such static check isn't possible in principle.

What I think we should do:

1. Revert the "fix" and distinguish between syntax errors (e.g. lines like = x, a=, a=<>, attributes and name duplication, mismatched parentheses and so on) and other errors, e.g. import errors.
2. Get the old behavior with the useful information back. Why does it not do that anymore? How can we get it back?
3. Split the parsing from the execution. Since this is Python and you cannot be sure what import of something does (I remember cases where you need to call something before a library is imported, as it cannot be changed afterwards), we should treat import as execution.

[tomasmcz]

@jindrahelcl Já si to rád fixnu sám, ale nejdřív si potřebuju s autory původního kódu ujasnit, co přesně to má dělat a jak je to vymyšlené, abych neopravoval něco, co není rozbité a nenadělal víc škody než užitku. K tomu slouží tahle možnost komentovat issues, což právě dělám, čímž na opravě pracuji a opravdu není potřeba mě popohánět připomínkami typu "solve one of your own issues".

[jindrahelcl]

@tomasmcz No jestli tvoje představa "práce" na projektu je, že budeš pouze kibicovat a netvořit žádnej skutečnej kód, až na pár commitů, který opravujou jednu řádku v readme, tak tu práci neodvádíš vůbec dobře, protože jediný, čeho postupně dosahuješ, je moje naprostá demotivace.

Už přes dva týdny sedí v seznamu na MTM několik issues, který čekaj na to, až na nich začneš něco dělat (#16, #38, #32, ...). Asi to pro tebe neni taková zábava, jako nepřetržitě čekovat něčí commity a strhávat na nich, co se dá, ale je potřeba s tim pohnout.

[jlibovicky]

@tomasmcz Your 3 point plan looks to me. A to, jakým stylem píšeš komentáře, je skutečně demotivující.