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-fix] Empty ignored trajectory queues, make sure queues don't overflow #3451

Merged
merged 3 commits into from
Feb 15, 2020

Conversation

ervteng
Copy link
Contributor

@ervteng ervteng commented Feb 14, 2020

Proposed change(s)

The current GhostTrainer subscribes to all trajectory queues (both learning and ghost) but doesn't empty the ghost queues as it doesn't use those trajectories for learning. This leads to a memory leak, at least until the queue is full (1000 trajectories, for Soccer it was about 5 GB).

This PR makes sure the GhostTrainer empties all queues (even if they are ignored) and also ensures queues are emptied even if they are filling faster than advance() is called (rare).

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added updated the changelog (if applicable)
  • I have added necessary documentation (if applicable) - Not Applicable
  • I have updated the migration guide (if applicable) - Not Applicable

Also make sure queues don't overflow
@ervteng ervteng requested a review from andrewcoh February 14, 2020 23:08
@ervteng ervteng changed the title Empty ignored trajectory queues, make sure queues don't overflow [bug-fix] Empty ignored trajectory queues, make sure queues don't overflow Feb 14, 2020
Copy link
Contributor

@andrewcoh andrewcoh left a comment

Choose a reason for hiding this comment

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

lgtm

@chriselion
Copy link
Contributor

Are we considering this for a hotfix release?

@ervteng
Copy link
Contributor Author

ervteng commented Feb 15, 2020

Are we considering this for a hotfix release?

Yes; probably would want to bundle it with other fixes though.

@ervteng ervteng merged commit e7dd212 into master Feb 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-emptyghostqueues branch February 15, 2020 00:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants