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

[hitl][hab-llm] New controller to work with habitat-llm Agents #1936

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

zephirefaith
Copy link
Contributor

@zephirefaith zephirefaith commented May 1, 2024

Motivation and Context

This PR creates LLMController based on SingleAgentBaselineController which houses basic init, reset and act code for calling habitat-llm agents.

Confused: Do I need to update SingleAgentAccessMgr class as well to be compatible with habitat-agents related to habitat-llm? The previously mentioned class just houses a lot of extra code which is useless in given setup. In habitat-llm setup the action is directly sent from policies running wherever habitat-llm code is running, without needing any policy setup or inference from SingleAgentAccessMgr class.
SingleAgentAccessMgr is also not needed, as inference comes from habitat-llm policies directly.

How Has This Been Tested

Types of changes

  • [Docs change] Addition or changes to the documentation
  • [Refactoring] Large changes to the code that improve its functionality or performance
  • [Dependency Upgrade] Upgrades one or several dependencies in habitat
  • [Bug Fix] (non-breaking change which fixes an issue)
  • [Development] A pull request that add new features to the habitat-lab task and environment codebase. Development Pull Requests must be small (less that 500 lines of code change), have unit testing, very extensive documentation and examples. These are typically new tasks, environments, sensors, etc... The review process for these Pull Request is longer because these changes will be maintained by our core team of developers, so make sure your changes are easy to understand!
  • [Experiment] A pull request that add new features to the habitat-baselines training codebase. Experiments Pull Requests can be any size, must have smoke/integration tests and be isolated from the rest of the code. Your code additions should not rely on other habitat-baselines code. This is to avoid dependencies between different parts of habitat-baselines. You must also include a README that will document how to use your new feature or trainer. You will be the maintainer of this code, if the code becomes stale or is not supported often enough, we will eventually remove it.

Checklist

  • My code follows the code style of this project.
  • I have updated the documentation if required.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes if required.

@zephirefaith zephirefaith requested a review from Ram81 May 1, 2024 18:17
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 1, 2024
@zephirefaith zephirefaith self-assigned this May 15, 2024
@zephirefaith zephirefaith marked this pull request as ready for review May 15, 2024 20:56
Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zephirefaith ! I did a first pass and raised some concerns.

Let me know if I can help!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add comments to config headers to explain the use cases covered by each of them.

max_episode_steps: 0
iterator_options:
# For the demo, we want to showcase the episodes in the specified order
shuffle: False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to pull those changes here.

# 2. What is the humanoidjoint_action? Do we need it here?
# - /habitat/task/[email protected]_1_humanoid_base_velocity: humanoid_base_velocity
# - /habitat/task/[email protected]_1_humanoidjoint_action: humanoidjoint_action
# - /habitat/task/[email protected]_1_base_velocity: base_velocity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xavierpuigf Would you know the answers to these?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. you don't need agent_1_base_velocity for the human, only agent_1_humanoid_base_velocity. The difference is that humanoid base velocity generates a human realistic motion whereas base velocity only moves the base (used to move the robot for instance).
  2. humanoidjoint_action sets the human joints so that the pose is realistic. You will never be callign it yourself, the agent_1_base_velocity sets that. But you sitll need to define it here

# - agent_1_dynamic_obj_start_sensor
# --- habitat-llm block
environment:
max_episode_steps: 750 # this is 20000 in habitat-llm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to double-check this.

  1. Is this enough to complete the longest tasks?
  2. What's the typical step count for a normal multiplayer run? (I'll get the data)
  3. What happens when this is done? We would need to call the appropriate code in HITL to trigger the next application state.


with habitat.config.read_write(self._config):
fix_config(self._config)
seed = 47668090
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this seed come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the default in all our code. We can move this to config. But same as what other experiments are using.

verbose=True,
)
if task_done:
print("Task Done")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that this would be the point where we hook up to HITL so that the agent can signal that its part of the task is done?

Would this also be called after the timeout? (max_episode_steps)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two conditions this would be called on:

  1. If # of planning steps > max planning steps threshold
  2. LLM reasons that the task is solved/can not be solved further

# distance_covered = max(
# 0, new_cum_distance_covered + offset - prev_cum_distance_covered
# )
# dist_diff = min(distance_to_walk, distance_covered)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need those changes to set humanoid walk speed. However, this may break other stuff.

Perhaps we could make animation-driven humanoid speed optional via config? If we do that, I'm afraid that it will be hard for other developers to figure out why the speed is limited.

Ideally, animation speed should be driven by locomotion speed.

@xavierpuigf Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, enabling/disabling this through config and adding comments both at config and code-level sounds like the least effort option to still maintain "other" uses of code.

Wdyt? Also, I do not completely understand this code layer so your help to add comments here would be perfect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! The fast solution is to disable animation. I have a way to remove the speed limitation but havent had time to get into it yet. If we want to have animation, I can work on integrating it next week.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not merge this as is though, it will break the functionalities for any other kind of motion

@@ -252,7 +255,9 @@ def calculate_walk_pose(
# The base_transform here is independent of transforms caused by the current
# motion pose.
obj_transform_base = look_at_path_T
forward_V_dist = forward_V * dist_diff * distance_multiplier
# HACK FROM MIKAEL
# forward_V_dist = forward_V * dist_diff * distance_multiplier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for this abomination :)

@0mdc
Copy link
Contributor

0mdc commented May 16, 2024

@zephirefaith You'll probably want to have this double-checked by someone who is familiar with the LLM configs so that we don't misstep here.

@zephirefaith zephirefaith changed the title New controller to work with habitat-llm Agents [hitl][hab-llm] New controller to work with habitat-llm Agents May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants