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

[Bug] Replace send_pickled in CARLA drivers #233

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mageofboy
Copy link
Contributor

send_pickle is deprecated in erdos. These changes replace the functionality of send_pickle by unpickling the message then using the standard send method. This is necessary for when running the pseudo-asynchronous simulator mode.

@pschafhalter pschafhalter changed the title fix send_pickle [Bug] Replace send_pickled in CARLA drivers Mar 17, 2022
@pschafhalter
Copy link
Member

Good catch, and thanks for the PR!

It looks like storing pickled messages in these operators is redundant -- we can just stores messages directly. Could you update the PR to make that change? Concretely:

  1. Rename _pickled_messages to _messages.
  2. Don't pickle the messages. Instead, do self._messages[msg.timestamp] = msg

@ICGog
Copy link
Contributor

ICGog commented Mar 18, 2022

We we're doing this in order to move the pickling of large sensor messages from the critical the critical path. If we stop pre-pickling messages, and sending them using the send_pickle method then the long pickling time will be accounted in the end-to-end response time, affecting experiments.

@pschafhalter
Copy link
Member

Hmmm, maybe we need to re-introduce this function in ERDOS.

I was under the impression that pickling a message took a negligible amount of time, but serializing a (1920, 1080, 3) numpy array takes around 15 ms on my laptop.

@ICGog
Copy link
Contributor

ICGog commented Mar 20, 2022

Yes, I think it would be good to re-introduce this function for now in order to ensure that the driver operators do not affect our experiments.

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.

3 participants