search

mila-iqia / babyai

BabyAI platform. A testbed for training agents to understand and execute language commands.
BSD 3-Clause "New" or "Revised" License
700 stars 146 forks source link

Bug in BOT Stack for PickupLoc #80

Open chitwansaharia opened 5 years ago

chitwansaharia commented 5 years ago

The BOT stack for BabyAI-PickupLoc-v0 contains a drop subgoal. It is due to the fact that every pickup subgoal automatically adds a drop subgoal. However, for PickupLoc, when the BOT needs to act done after finishing all the subgoals will end up failing the mission, as it would have dropped the object already. @rizar

rizar commented 5 years ago

Thanks does sound like a bug. I guess the bot should behave differently when it needs to pick smth up in the end of the execution and not in the middle.

Would you like to try fixing it?

On Sun, 18 Aug 2019 at 17:25, Maxime Chevalier-Boisvert < notifications@github.com> wrote:

Assigned #80 https://github.com/mila-iqia/babyai/issues/80 to @rizar https://github.com/rizar.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/mila-iqia/babyai/issues/80?email_source=notifications&email_token=AAE7YYS2WHS54PSF6TMU5LTQFG44ZA5CNFSM4IMUDUCKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTDO527Q#event-2564676990, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE7YYWF6H542V3IUVRAQ5TQFG44ZANCNFSM4IMUDUCA .

maximecb commented 5 years ago

An important thing to check, after fixing the bug, is that the correctness and performance of the bot is not affected. See the eval_bot script.

@chitwansaharia is this bug blocking you right now?

chitwansaharia commented 5 years ago

Sure. I will try fixing the bug, and check the correctness and performance of the bot afterwards. @maximecb I made a temporary fix to serve my purpose. I removed the drop subgoal manually while generating demos for PickupLoc. So it is not blocking me currently.

chitwansaharia commented 5 years ago

Thanks

Even in the instructions like Pickup the blue ball and go to the red ball, you still need to remove the drop subgoal even if the pickup is not the last thing that needs to be done. A possible solution I can think of is post-processing stack inserting drop subgoal between every two Pickups (inserting it just before latter Pickup subgoal). Do you think this approach is okay ? @rizar

akshay-sharma1995 commented 5 years ago

Hey @chitwansaharia were you able to fix this bug? I have an older version of Babyai and I am getting Assertion Error assert self.bot.mission.carrying if I try to run the expert for PickupLoc using the learner's actions. This is coming from the Drop Subgoal, which I am guessing because of the issue you mentioned. Before updating to the latest version I just wanted to know whether the bug was fixed or not. If not, can you help me with the workaround you have mentioned about removing the drop subgoal, like when should I be removing it from the subgoals stack.

maximecb commented 5 years ago

@chitwansaharia ?

chitwansaharia commented 5 years ago

Hi @akshay-sharma1995 I have not pushed the solution yet. Although, the possible workaround for PickupLoc specifically is just to remove the drop subgoal from the BOT stack. It is just a list of subgoals, and you can easily remove the Drop Subgoal from that list.

Hey @chitwansaharia were you able to fix this bug? I have an older version of Babyai and I am getting Assertion Error assert self.bot.mission.carrying if I try to run the expert for PickupLoc using the learner's actions. This is coming from the Drop Subgoal, which I am guessing because of the issue you mentioned. Before updating to the latest version I just wanted to know whether the bug was fixed or not. If not, can you help me with the workaround you have mentioned about removing the drop subgoal, like when should I be removing it from the subgoals stack.

chitwansaharia commented 5 years ago

@rizar Please look at the solution idea I proposed before. If that sounds good, I will go ahead and create the PR. Thanks!

akshay-sharma1995 commented 5 years ago

@chitwansaharia, @maximecb I tried this fix. For pickuploc env I removed the DropSubgoal, but now I am getting an assertion error for the PickupSubGoal assert not self.bot.mission.carrying which means the bot is already carrying something. Just to clarify I am trying to run the Bot along with a trained agent and using the agent's action to update the Bot state. According to the comment on L554 in the replan function in bot.py action_taken The last action that the agent took. Can be None, in which case the bot assumes that the action it suggested was taken (or that it is the first iteration). I should be able to pass the action taken by the trained agent to the Bot and it should be able to replan the path. This setup works for the other two envs i tested (GoToLocal-v0 and GoToObJMaje-v0) but it is giving me the above mentioned error in PickUpLoc-v0 and any other env which has PickupSubgoal. Am I passing the trained agent's action to the Bot in the correct way?

chitwansaharia commented 5 years ago

@akshay-sharma1995, can you post the relevant demo code snippet, so that I can run it and get more idea about the problem you are facing ?

rizar commented 5 years ago

Sorry for the delay, let me join the discussion.

@chitwansaharia , why do we have to remove DropSubgoal from the stack that correspondsPickup the blue ball and go to the red ball. What harm does DropSubgoal do in this case?

akshay-sharma1995 commented 5 years ago

Hey @chitwansaharia, sorry for the delay. I am trying to run an expert along with a trained learner while evaluation. I made the relevant changes in the babyai/babyai/evaluate.py file. I have attached the code as a pastebin over here: https://pastebin.com/embed_js/FPhi7RWV I have added made the relevant changes in the ManyEnvs class and batch_evaluate function. You can run it by using the default evaluation script as mentioned in the babyai docs. I am able to run it for the GoToLocal-v0 env and the GoToObjMaje-v0 env, but it gives error with the PickupLoc-v0 env. I am assuming there will be same issues with any env with a Pickup subgoal.