[WIP] Feat/oracle cache #64

naterush · 2017-10-18T06:45:42Z

Work started on this issue: #59

Other tests are passing, so it appears I haven't broken anything too badly. The code could use some serious testing and cleanup though :)

@djrtwo would love to talk about architecture trade-offs here. Also, would love to get some stats about performance improvements :)

naterush · 2017-10-18T06:49:21Z

casper/safety_oracles/oracle_manager.py

+
+                            # if it is a free block
+                            if message.sequence_number > seq_number:
+                                # NOTE: preferably, would not do this for all new messages - rather just latest free block?


This could add some significant efficiency gains - I think. Will take a crack at this tomorrow (not too hard I don't think, just storing new messages in a dict + list, I think).

naterush · 2017-10-19T10:55:34Z

Ok, so the root of the bug above is (I'm 95% sure) the fact that I don't search through messages later than last_checked_messages even when I add a new estimate.

The issue here is that there are very likely viewables here that I am missing because of this (massive oversight - I know!). There should be an easy fixup tomorrow (and should fix the remaining bugs in all the oracles that result from these missing viewables).

djrtwo · 2017-10-19T15:45:29Z

Where the tests at brah?
I wanna see the bug but I guess it's from manual testing

Holler if you need another set of eyes. Going to leave it to you until you say otherwise for review/help

naterush · 2017-10-19T18:25:31Z

Yep, manual testing and many assert statements. Will be able to get some tests up tonight with the fixes hopefully :~)

Interestingly, though this is horribly bugged, none of our previous test cases catch it :) We need to bulk up on some safety oracle tests.

djrtwo · 2017-10-19T18:31:00Z

Write some test cases before you fix it! It's good to see those red test cases and fix them. Then we'll know we tested this previously untested set of cases.

…

On Thursday, October 19, 2017, Nate Rush ***@***.***> wrote: Yep, manual testing and many assert statements. Will be able to get some tests up tonight with the fixes hopefully :~) Interestingly, though this is horribly bugged, none of our previous test cases catch it :) We need to bulk up on some safety oracle tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#64 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABXf-9C1PmogM_b9NILZ0gV0VYUaE6ssks5st5QcgaJpZM4P9Oqe> .

naterush · 2017-10-19T19:08:58Z

@djrtwo good call. Will do.

djrtwo

Looks good! A few things

I'd like to see oracle_class passed into OracleManager. This will open up more easily testing against the Oracles. Consider getting some of the tests to run against multiple Oracles when you add this.
Break down some of the big functions in OracleManager to readability.
Use a linting tool. Either integrate something into your editor that warns you or run pylint from the commandline. We're working on getting the entire repo linted up and once we do, having no linting errors will be part of CircleCI passing.
testing
- Minimum unit tests doing at least some sanity checking on OracleManager initialization and basic functions. I know safety tests with testlang cover the heavy stuff. I want some test somewhere to at least break if we change the OracleManager API.
- Run safety tests against each Oracle.

djrtwo · 2017-10-20T17:27:07Z

casper/safety_oracles/oracle_manager.py

+        self.last_checked_messages = dict()
+
+
+    def update_viewables(self):


I'd like to see this broken into sub methods (prefixed with "_") -- something like _track_new_viewables, _remove_outdated_viewables, and _update_last_checked_messages

I want to be able to look at update_viewables, read the two or three submethods it calls and know exactly what is going on from a high level just by reading those method names.

djrtwo · 2017-10-20T17:58:24Z

casper/safety_oracles/oracle_manager.py

+
+class OracleManager:
+
+    def __init__(self, view, validator_set, safety_threshold):


Should probably take a param oracle_class so the user can pass in CliqueOracle, TuranOracle, etc for the manager to use.

One fun thing from this would be the ability to instantiate different validators with different oracles...

Good call. Also, the ability for validators to have different safety_thesholds now :)

djrtwo · 2017-10-20T18:01:41Z

tests/test_safety_oracle.py

+
+def test_cache_checks_all_possible_viewables(test_lang_runner):
+    test_string = ''
+    for i in range(100):


This needs a comment or something. Very unreadable

djrtwo · 2017-10-20T18:04:14Z

tests/test_safety_oracle.py

+            test_string += 'U' + str(j) + '-' + str(i) + ' ' + 'U' + str(j) + '-' + str(i + 100) + ' '
+    test_string = test_string[:-1]
+
+    weights = {0: 10, 1: 9.5, 2: 8.32, 3: 7.123, 4: 6.02245}


What's the explanation on these weights? are they important to the test? The complexity of the test_string leaves me unsure.

If the weights don't matter, consider doing a "parameters" header on this with 2 to 3 different weights defs

djrtwo · 2017-10-20T18:05:32Z

casper/safety_oracles/oracle_manager.py

@@ -0,0 +1,141 @@
+from casper.safety_oracles.clique_oracle import CliqueOracle


If you pass in oracle_calss to __init__, you won't need to do any of these imports here.

…ized_messages'

djrtwo · 2017-10-23T02:20:22Z

@naterush Merged in simulationrunner changes

…r running tests

…bc-casper into feat/oracle_cache

naterush · 2017-10-23T16:12:49Z

casper/safety_oracles/oracle_manager.py

@@ -14,17 +14,17 @@ def __init__(self, view, validator_set, safety_threshold):
    def update_viewables(self):
        """For each estimate being tracked, finds new and removes old viewables based on view"""
        # finds all new viewables, for each validator, for each estimate we are keeping track of
-        self._track_new_viewables()
+        val_with_new_messages = self._track_new_viewables()


Returning this from the track new viewables function is hack-y and unnecessary. The clean-up I was going to start on just breaks these into functions that do what they say they do.

For example, it might be better to have the for estimate in self.viewables_for_estimate: loop in the update_viewables function - and then pass in the estimate to each of the respective functions. It's not a hard change either way but would love to hear your thoughts.

Very much agree. Yeah, that makes sense. Do everything that needs to be done for a given estimate and then move on to the next estimate

naterush · 2017-10-25T07:21:17Z

Once we settle on the best oracle_manager design, I think the best option is probably too close this PR, rebase onto a new branch, and then restart. Either that or clean up the commit history, b/c things are quite messy :)

naterush · 2017-10-26T20:18:32Z

Closing this for now. Not a priority for devcon - and it's in a state of disrepair as of now. Will reopen when after.

naterush added 3 commits October 18, 2017 02:17

add oracle_manager for caching of viewables

3c090c9

update oracles to work with passed viewables

36db01d

update casper and validator to use oracle_manager

5e06099

naterush commented Oct 18, 2017

View reviewed changes

naterush added 2 commits October 19, 2017 20:42

fixup bugs in the oracle manager and oracles

580740b

add test to make sure validators never detect safety incorrectly

67b52fa

naterush force-pushed the feat/oracle_cache branch from 9279f6f to 67b52fa Compare October 20, 2017 03:42

djrtwo suggested changes Oct 20, 2017

View reviewed changes

naterush added 4 commits October 20, 2017 17:28

make oracle manager take some safety oracle class when checking safety

2567e0f

make update_viewables break work into functions

0903f36

lint oracle_manager

769ce70

comment and rename safety_oracle tests, start oracle_manager tests

7293950

naterush force-pushed the feat/oracle_cache branch from 103dfe5 to 7293950 Compare October 21, 2017 00:30

djrtwo added 4 commits October 22, 2017 20:00

Merge branch 'master' into feat/oracle_cache

bebda29

lints

d55edf4

Merge branch 'master' into feat/oracle_cache

c00f3cf

[feat/oracle_class] reduce rounds on 'test_no_final_leads_to_no_final…

987ed86

…ized_messages'

naterush added 5 commits October 23, 2017 01:13

return the test_lang object, so we can assert things about state afte…

7c0f2ed

…r running tests

add tests for the oracle manager

f93e0de

Merge branch 'feat/oracle_cache' of https://github.com/karlfloersch/c…

4bcff83

…bc-casper into feat/oracle_cache

move oracle_manager

8337ad5

quick optimization

14e3a9c

naterush commented Oct 23, 2017

View reviewed changes

breakup functions, make things a bit more comprehensible

9bcfa39

naterush closed this Oct 26, 2017

naterush deleted the feat/oracle_cache branch February 14, 2018 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feat/oracle cache #64

[WIP] Feat/oracle cache #64

naterush commented Oct 18, 2017

naterush Oct 18, 2017 •

edited

Loading

naterush commented Oct 19, 2017

djrtwo commented Oct 19, 2017

naterush commented Oct 19, 2017

djrtwo commented Oct 19, 2017 via email

naterush commented Oct 19, 2017

djrtwo left a comment •

edited

Loading

djrtwo Oct 20, 2017

djrtwo Oct 20, 2017

djrtwo Oct 20, 2017

naterush Oct 20, 2017

djrtwo Oct 20, 2017

djrtwo Oct 20, 2017

djrtwo Oct 20, 2017

djrtwo Oct 20, 2017

djrtwo commented Oct 23, 2017

naterush Oct 23, 2017

djrtwo Oct 23, 2017

naterush commented Oct 25, 2017

naterush commented Oct 26, 2017

		self.last_checked_messages = dict()


		def update_viewables(self):


		class OracleManager:

		def __init__(self, view, validator_set, safety_threshold):

		@@ -0,0 +1,141 @@
		from casper.safety_oracles.clique_oracle import CliqueOracle

[WIP] Feat/oracle cache #64

[WIP] Feat/oracle cache #64

Conversation

naterush commented Oct 18, 2017

naterush Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

naterush commented Oct 19, 2017

djrtwo commented Oct 19, 2017

naterush commented Oct 19, 2017

djrtwo commented Oct 19, 2017 via email

naterush commented Oct 19, 2017

djrtwo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naterush commented Oct 25, 2017

naterush commented Oct 26, 2017

naterush Oct 18, 2017 •

edited

Loading

djrtwo left a comment •

edited

Loading