-
Notifications
You must be signed in to change notification settings - Fork 77
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
Adding assignment-unit cache to singleton DB #441
Conversation
sandbox: bool = True, | ||
) -> str: | ||
""" | ||
Create a new unit with the given index. Raises EntryAlreadyExistsException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does throwing the exception happen in the super class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
==========================================
- Coverage 66.05% 65.95% -0.10%
==========================================
Files 78 78
Lines 7105 7118 +13
==========================================
+ Hits 4693 4695 +2
- Misses 2412 2423 +11
Continue to review full report at Codecov.
|
mephisto/operations/operator.py
Outdated
@@ -306,7 +306,7 @@ def _track_and_kill_runs(self): | |||
tracked_run.architect.shutdown() | |||
tracked_run.task_launcher.shutdown() | |||
del self._task_runs_tracked[task_run.db_id] | |||
time.sleep(2) | |||
time.sleep(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this as hardcoded constant (probably all caps) at the top of the file or in a constants file (if you are using one). Specially having a descriptive name helps someone who may want to debug later see what is readily available for tweaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Jack. Can't wait to run with new changes 😃
Overview
Assignment.get_units
is currently the largest remaining call in the Mephisto profile when running withMephistoSingletonDB
This PR looks to remove this chunk by adding a cache for this specific call.Implementation
Creates a dict in the
MephistoSingletonDB
to hold a mapping between the givenassignment_id
s queried and the returned list ofUnit
s. Also adds a wrapper aroundnew_unit
to ensure that if a new unit is added to a givenassignment_id
we clear that cached value. Considering we're within a singleton, this is sufficient to ensure that the local units will always be up-to-date.Testing
In python shell:
The second call to
get_assignment_statuses()
was instant, where the first took about a second.