Issue1029

Title Unclear error message when "define" keyword is missing
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, victor.paleologue
Assigned To Keywords
Optional summary
When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong.

See issue220.

This issue is associated to this GitHub PR: https://github.com/aibasel/downward/pull/59

Created on 2021-07-19.08:10:33 by victor.paleologue, last changed by malte.

Summary
When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong.

See issue220.

This issue is associated to this GitHub PR: https://github.com/aibasel/downward/pull/59
Messages
msg11527 (view) Author: malte Date: 2024-01-18.15:16:54
We merged the pull request for issue1030, which also covers this issue.

I'm marking this as resolved, but more things could be done, as discussed in issue1030.

Thanks for reporting this and for your patience, Victor! I've also added you to the README as a contributor as discussed.
msg11474 (view) Author: victor.paleologue Date: 2023-10-23.14:35:55
It works for me.
msg11473 (view) Author: malte Date: 2023-10-23.14:07:30
Hi Victor, we'll use the new pull request which doesn't include your commit, but does include the functionality change. We should mention you as a contributor in the README file because you were involved in this change. There's no email addresses recorded in the README file, so if it's understood that this is a personal contribution and not one by the company, we'll be fine. Let us know if this works for you.
msg11472 (view) Author: victor.paleologue Date: 2023-10-23.13:11:39
Ok, I understand.
I think I can safely rewrite this commit with a personal address if it helps, let me know.
msg11469 (view) Author: malte Date: 2023-10-23.11:44:08
Sorry, that last comment got cut off. What I wanted to add was that the difficulty with copyright (see what is mentioned below) is something that makes merging changes like these difficult in general, especially when they are associated with commercial email addresses.
msg11468 (view) Author: malte Date: 2023-10-23.11:39:47
Hi Victor,

we have had a pull request with completely rewritten error checking for the translator for some time. It supersedes and conflicts with your two pull requests, so we were planning to integrate that one instead as mentioned in my previous message. It has been reviewed and tested, but last time I checked it got stuck due to some (small) unaddressed code review comments. Part of the delay is that the person that worked on it left the group, so there has been some confusion about who will take this to completion.

We'll prioritize landing that patch soon. I'm sorry for your negative experience. As I explained two years ago, 

Best,
Malte
msg11457 (view) Author: victor.paleologue Date: 2023-10-17.11:16:11
There has always been a pull request for this: https://github.com/aibasel/downward/pull/59

It is just about adding some message to an existing assertion.
It is still applicable.

I don't know where the refactoring is at.
This change is so trivial it's not worth waiting for 2 years to merge it.

If there is something wrong about the PR, I'm not sure I have the patience to deal with it anymore. In that case you can close the PR and the issue, I won't mind.
msg11063 (view) Author: malte Date: 2023-03-24.17:44:10
Hi Victor, I hope that your patch is covered by the much more extensive error checking that we worked on in February, but I am unclear of the status of that work.

To those reading here: is there an issue number for this? Is there a pull request? Is the process stuck, and if yes, can we make it unstuck?
msg11061 (view) Author: victor.paleologue Date: 2023-03-22.11:15:24
Hello, any news? Is the error handled properly on master now? Should I drop my PR?
msg10979 (view) Author: malte Date: 2023-02-06.10:55:20
Yes, we have been working on the error messages for the last two weeks. Expect some progress soon.
msg10978 (view) Author: victor.paleologue Date: 2023-02-06.10:54:44
Hello, any news?
msg10872 (view) Author: florian Date: 2022-12-05.11:28:50
Let me repeat what I wrote on the pull request, so the tracker stays up to date: We said that we'll consider pulling something about error checking PDDL in the next sprint in January. If we do, this will also cover this issue, if not, we can merge the current pull request as a stopgap.
msg10433 (view) Author: victor.paleologue Date: 2021-09-01.14:27:12
Thanks for explaining. I'll remove the change log entry, since this change is so minor. Thus we rule out the communication issue.

For the legal issue, there is nothing I can do to help. I thought having also the MIT-like license and being owner of the brand gave you that security.

What would be your final conclusion on this PR, then?
https://github.com/aibasel/downward/pull/59
msg10432 (view) Author: malte Date: 2021-09-01.14:16:05
It is not the integration process as such, it is more that we generally like to approach related improvements in larger chunks for communication reasons. A changelog entry is also a communication to the user base. There are currently probably at least 20 ways in which the error reporting for invalid PDDL should be improved, and we'd rather communicate this to the users in one or a few communications of the form "We finally improved the handling of PDDL. Hope you like the change!" rather than with 20 separate changelog entries.

There's also the question of what accepting externally contributed pull requests like these means for the contributors file and the legal status of the project. Even for GPL-ed projects like these, authorship is important. For example, currently in research proposals sent to public funding agencies, we can say "We're bringing into the project the Fast Downward planner, for which we hold the rights", and this is something that helps us get funding for our research. Once we don't own the planner any more, that becomes problematic. (GPL is all well and good, but for example for EU funding proposals there is a huge bureaucratic process where legal teams need to be kept happy. It's not great, but it's the way it is.)

Things like these can of course often be arranged, but it's more worth the effort for cases where it comes with substantial progress.
msg10426 (view) Author: victor.paleologue Date: 2021-09-01.09:36:39
Hi Malte,
I understand that stance, however let us get into the specifics.
The PR https://github.com/aibasel/downward/pull/59 just adds a message to an existing assertion. The code is there, though you there is something to fix there, the issue is there, the changelog is there. What is it in your integration process that would really cost so much that compromises accepting the PR?
msg10423 (view) Author: malte Date: 2021-08-31.12:47:37
Hi Victor, this is an improvement in the right direction, but every change, even a small one, takes some work to integrate, and hence I'd prefer to improve the error reporting in somewhat larger steps than this. Assertions are not really the correct way to report invalid inputs, we're only using them as a crutch until we have something better. I would prefer to move on directly to something that we'd be happier with in the medium term rather than to a better version of a stop-gap measure.

BTW, the pull request violates our style guide regarding line lengths.
msg10420 (view) Author: malte Date: 2021-08-30.19:51:32
See issue220.
msg10419 (view) Author: malte Date: 2021-08-30.19:49:51
Hi Victor, thanks for reporting! This is parting of a more general problem that PDDL error reporting is currently very poor. I'll try to have a look when I have some cycles to spare, but not sure if this will happen in the near future. :-( If this sits idle for several months from now, feel free to ping me (by answering here on the tracker or by email).
msg10366 (view) Author: victor.paleologue Date: 2021-07-20.10:16:11
New corrected PR: https://github.com/aibasel/downward/pull/59
msg10365 (view) Author: victor.paleologue Date: 2021-07-19.08:10:32
Related PR: https://github.com/aibasel/downward/pull/43
History
Date User Action Args
2024-01-18 15:16:54maltesetstatus: in-progress -> resolved
messages: + msg11527
2023-10-23 14:35:55victor.paleologuesetmessages: + msg11474
2023-10-23 14:07:30maltesetmessages: + msg11473
2023-10-23 13:11:39victor.paleologuesetmessages: + msg11472
2023-10-23 11:44:08maltesetmessages: + msg11469
2023-10-23 11:39:47maltesetmessages: + msg11468
2023-10-17 11:16:11victor.paleologuesetmessages: + msg11457
2023-03-24 17:44:10maltesetmessages: + msg11063
2023-03-22 11:15:24victor.paleologuesetmessages: + msg11061
summary: When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong. See issue220. -> When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong. See issue220. This issue is associated to this GitHub PR: https://github.com/aibasel/downward/pull/59
2023-02-06 10:55:20maltesetmessages: + msg10979
2023-02-06 10:54:44victor.paleologuesetmessages: + msg10978
2022-12-22 12:30:04victor.paleologuesetmessages: - msg10904
2022-12-22 12:25:09victor.paleologuesetmessages: + msg10904
2022-12-05 11:28:50floriansetnosy: + florian
messages: + msg10872
2021-09-01 14:27:13victor.paleologuesetmessages: + msg10433
2021-09-01 14:16:05maltesetmessages: + msg10432
2021-09-01 09:36:39victor.paleologuesetmessages: + msg10426
2021-08-31 12:47:38maltesetmessages: + msg10423
2021-08-30 19:51:52maltesetassignedto: victor.paleologue ->
2021-08-30 19:51:32maltesetmessages: + msg10420
summary: When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong. -> When submitting a wrong PDDL domain lacking the "define" statement to Fast Downward, the translator gives very little information about what is wrong. See issue220.
2021-08-30 19:49:51maltesetnosy: + malte, jendrik
messages: + msg10419
2021-07-20 10:18:36victor.paleologuesetmessages: - msg10367
2021-07-20 10:18:25victor.paleologuesetmessages: + msg10367
2021-07-20 10:16:11victor.paleologuesetmessages: + msg10366
2021-07-19 08:10:33victor.paleologuecreate