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

Env package #44

Merged
merged 71 commits into from
Apr 13, 2024
Merged

Env package #44

merged 71 commits into from
Apr 13, 2024

Conversation

zhangk1551
Copy link
Contributor

@zhangk1551 zhangk1551 commented Apr 9, 2024

Merged the torchdrivesim master into first-release-env and cleanup the map and traffic control related code. The code for the stop_sign violation is not in this PR, because it's not used in torchdriveenv now.

Since most related changes are already checked in the master, this PR is quite light-weighted.

I moved the resources folder under the torchdrivesim folder, because when I packaged the lib via pip install "torchdrivesim @ git+https://github.com/inverted-ai/torchdrivesim.git@first-release-env", it's a workable way to include the resources data to the library. Let me know if there is a better way to package the resources.

After this PR is merged, I will change the git submodule in the torchdriveenv as master. Once the torchdrivesim master is released to pip, it can be installed without a specific branch in torchdriveenv.

@zhangk1551 zhangk1551 changed the base branch from master to first-release-env April 9, 2024 00:23
@zhangk1551 zhangk1551 changed the base branch from first-release-env to master April 9, 2024 00:24
@@ -30,19 +32,84 @@ def iai_initialize(location, agent_count, center=(0, 0), traffic_light_state_his
return agent_attributes, agent_states, response.recurrent_states


def iai_drive(location: str, agent_states: Tensor, agent_attributes: Tensor, recurrent_states: List,
traffic_lights_states: Optional[dict] = None):
def iai_conditional_initialize(location, agent_count, agent_attributes=None, agent_states=None, recurrent_states=None, center=(0, 0), traffic_light_state_history=None):
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 might also be used by code outside of torchdriveenv if needed, so I keep it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is specifically what I would like to avoid. I don't want this version to proliferate, so let's move it to torchdriveenv. If it stays here, it will be harder to update later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@@ -62,12 +129,15 @@ class IAIWrapper(NPCWrapper):
"""
def __init__(self, simulator: SimulatorInterface, npc_mask: TensorPerAgentType,
recurrent_states: List[List], locations: List[str], rear_axis_offset: Optional[Tensor] = None,
car_sequences: Optional[Dict[int, List[List[float]]]] = None,
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 used by torchdriveenv to set parked car, and can also be used to load preset logs.

Potentially, it can be replaced by ReplayWrapper with careful design, but currently, let's leave it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can accept that, but can you at least make it replay_states (BxAxTxSt float) and replay_mask (BxA or BxAxT bool) so that we use tensors consistently, rather than resorting to ad-hoc formatted dictionaries?

@zhangk1551 zhangk1551 requested a review from adscib April 9, 2024 00:59
Copy link
Contributor

@adscib adscib left a comment

Choose a reason for hiding this comment

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

Almost there. Do you still need that resources folder at all? We now install lanelet2 from pip, and the .osm files and meshes are available using the new map files format if needed.

pyproject.toml Outdated
@@ -22,7 +22,7 @@ dependencies = [
"scipy",
"imageio",
"torch>=1.10.1",
"invertedai",
"invertedai==0.0.16",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"invertedai==0.0.16",
"invertedai",

I think this will work with older versions of invertedai now as well. We can set this constraint in torchdriveenv instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 1 to 4
from invertedai.error import InvertedAIError


class InitializationFailedError(InvertedAIError):
Copy link
Contributor

Choose a reason for hiding this comment

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

If we inherit from InvertedAIError, I would move this to torchdrivesim.behavior.iai.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found this class was not only used in the iai related cases (

from torchdrivesim.behavior.common import InitializationFailedError
,
from torchdrivesim.behavior.common import InitializationFailedError
). Let me restore the change.

@@ -30,19 +32,84 @@ def iai_initialize(location, agent_count, center=(0, 0), traffic_light_state_his
return agent_attributes, agent_states, response.recurrent_states


def iai_drive(location: str, agent_states: Tensor, agent_attributes: Tensor, recurrent_states: List,
traffic_lights_states: Optional[dict] = None):
def iai_conditional_initialize(location, agent_count, agent_attributes=None, agent_states=None, recurrent_states=None, center=(0, 0), traffic_light_state_history=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

That is specifically what I would like to avoid. I don't want this version to proliferate, so let's move it to torchdriveenv. If it stays here, it will be harder to update later.

agent_attribute_list = response.agent_attributes + outside_agent_attributes
agent_state_list = response.agent_states + outside_agent_states
recurrent_state_list = response.recurrent_states + outside_recurrent_states
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is overly broad and will capture things like keyboard interrupts and out of memory exceptions, which we don't want. You probably want to catch InvertedAIError and maybe a few others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return agent_attributes, agent_states, recurrent_state_list


def iai_drive(location: str, agent_states: Tensor, agent_attributes: Tensor, recurrent_states: List, traffic_lights_states: Dict = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind fixing the type signature while you're editing this function? It should specify the list element type for recurrent_state, make traffic_lights_states Optional, and specify return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 111 to 112
except Exception as e:
raise e
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do anything and can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -62,12 +129,15 @@ class IAIWrapper(NPCWrapper):
"""
def __init__(self, simulator: SimulatorInterface, npc_mask: TensorPerAgentType,
recurrent_states: List[List], locations: List[str], rear_axis_offset: Optional[Tensor] = None,
car_sequences: Optional[Dict[int, List[List[float]]]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can accept that, but can you at least make it replay_states (BxAxTxSt float) and replay_mask (BxA or BxAxT bool) so that we use tensors consistently, rather than resorting to ad-hoc formatted dictionaries?

traffic_light_controller: Optional[TrafficLightController] = None, traffic_light_ids: Optional[List[int]] = None):
super().__init__(simulator=simulator, npc_mask=npc_mask)

self._locations = locations
self._npc_predictions = None
self._recurrent_states = recurrent_states
self._car_sequences = car_sequences
self._iai_timestep = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._iai_timestep = 0
self._replay_timestep = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@Ruishenl
Copy link
Contributor

Probably not related to this PR, but i think we should also add a gif from the documentation page to the README so people can have a rough idea how does everything look in general.

Also, we mentioned in the README the pypi version of this package comes with the OpenCV renderer, but we haven't made any official release to PyPI since last February. So a simple pip install will not get user the latest change from master

Copy link
Contributor Author

@zhangk1551 zhangk1551 left a comment

Choose a reason for hiding this comment

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

I removed the lanelet2 dependency, but I didn't quite get the ideas about how the map files can be packaged to pip. @adscib

pyproject.toml Outdated
@@ -22,7 +22,7 @@ dependencies = [
"scipy",
"imageio",
"torch>=1.10.1",
"invertedai",
"invertedai==0.0.16",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 1 to 4
from invertedai.error import InvertedAIError


class InitializationFailedError(InvertedAIError):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found this class was not only used in the iai related cases (

from torchdrivesim.behavior.common import InitializationFailedError
,
from torchdrivesim.behavior.common import InitializationFailedError
). Let me restore the change.

@@ -30,19 +32,84 @@ def iai_initialize(location, agent_count, center=(0, 0), traffic_light_state_his
return agent_attributes, agent_states, response.recurrent_states


def iai_drive(location: str, agent_states: Tensor, agent_attributes: Tensor, recurrent_states: List,
traffic_lights_states: Optional[dict] = None):
def iai_conditional_initialize(location, agent_count, agent_attributes=None, agent_states=None, recurrent_states=None, center=(0, 0), traffic_light_state_history=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

agent_attribute_list = response.agent_attributes + outside_agent_attributes
agent_state_list = response.agent_states + outside_agent_states
recurrent_state_list = response.recurrent_states + outside_recurrent_states
except Exception:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return agent_attributes, agent_states, recurrent_state_list


def iai_drive(location: str, agent_states: Tensor, agent_attributes: Tensor, recurrent_states: List, traffic_lights_states: Dict = None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 111 to 112
except Exception as e:
raise e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

traffic_light_controller: Optional[TrafficLightController] = None, traffic_light_ids: Optional[List[int]] = None):
super().__init__(simulator=simulator, npc_mask=npc_mask)

self._locations = locations
self._npc_predictions = None
self._recurrent_states = recurrent_states
self._car_sequences = car_sequences
self._iai_timestep = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@adscib adscib left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few nitpicks left.

torchdrivesim/resources/maps/carla_Town06/metadata.json Outdated Show resolved Hide resolved
torchdrivesim/resources/maps/carla_Town07/metadata.json Outdated Show resolved Hide resolved
import invertedai
from invertedai.common import AgentState, AgentAttributes, Point
agent_attributes = [AgentAttributes(length=at[0], width=at[1], rear_axis_offset=at[2]) for at in agent_attributes]
agent_states = [AgentState(center=Point(x=st[0], y=st[1]), orientation=st[2], speed=st[3]) for st in agent_states]
seed = random.randint(1, 10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this preferred to leaving seed as None? If you want reproducibility after setting global seed, that makes sense, but in that case do it consistently in both drive and initialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's for reproducibility. It's set in the drive and conditional_initialize, because torchdriveenv doesn't use the original initialize.

torchdrivesim/behavior/iai.py Outdated Show resolved Hide resolved
@zhangk1551 zhangk1551 merged commit e93c34e into master Apr 13, 2024
4 checks passed
@zhangk1551 zhangk1551 deleted the env-package branch April 13, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants