2. Fix blacklistpolicy and add query #92
Conversation
|
Could we put the query in the base Policy class? So we could get the result even if we are not using ABL policy. |
|
@jermainewang This query is designed for blacklist rule. The rules won't be loaded before first switch to ABL. |
lryta
changed the title
| if ns_path not in cls._rules or name not in cls._rules[ns_path]: | ||
| return 'No rule for {} is found in {}.'.format(name, ns_path) | ||
| else: | ||
| from tabulate import tabulate |
There was a problem hiding this comment.
Please make sure this dependency works under python3
There was a problem hiding this comment.
tabulate works in python3. No worries.
| self._rules.setdefault(nspace, {}) | ||
| self._rules[nspace].setdefault(name, []) | ||
| self._hash.setdefault(nspace, {}) | ||
| self._hash[nspace].setdefault(name, set()) |
There was a problem hiding this comment.
Use collections.defaultdict to replace these?
There was a problem hiding this comment.
I try to fix, but it seems that YAML will use some chars to record since it is a class different from ordinary dict. The current solution don't need to record extra chars or do explicit conversion between defaultdict and dict when saving the dict into YAML file.
| @@ -253,3 +268,37 @@ def _get_arg_rule_key(self, args, kwargs): | |||
| arg_key = [self._get_type_signiture(x) for x in args] | |||
| kwarg_key = sorted(kwargs.keys()) | |||
| return '-'.join(arg_key) + '+' + '-'.join(kwarg_key) | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
This is a little bit weird here. You take a cls argument and use the _rules belongs to it. This means one might create multiple policy objects and they have different rules associate with it, which is quite counter-intuitive. I think it is better to have only one policy object for one policy type. When we switch the policy, we are not constructing an object, but instead:
policy.set_global_policy("only_numpy")
# policy.set_global_policy("auto_blacklist")
# policy.set_global_policy("prefer_mxnet")This will also avoid importing the policy python file. What do you think?
There was a problem hiding this comment.
@jermainewang In fact they cannot create different rules. The rule is associated with class instead of objects. Ending with this solution is a compromise of all existing features like with statement of a policy object. I've emphasized in the doc.
There was a problem hiding this comment.
I think with statement is also not recommended based on our design of ABL.
There was a problem hiding this comment.
The question is still here. If all the object's states are associated with the class, then the object itself is meaningless. We should instead have API like:
policy.AutoBlackListPolicy.query(name)
|
Let's move discussions to #93 . |
Fix namespace problem and add query for blacklistpolicy.
Passed all tests.