Refactor resolver into a tree of callable objects, or partially evaluated #95
Conversation
|
Yay, still green tests, coverage around the same as it used to, and +285 - 280 lines of code in the patch. @spookylukey did much of the work here, tbh. Changing a resolver with existing test coverage is a two-day task, if you get pointed at the problem to solve, too. Next steps are for me to actually review this patch myself, I didn't do that yet ;-) |
|
@spookylukey, I've done a round of self-review now (bad TV programme tonight), I'd love to get your feedback on this. |
There was a problem hiding this comment.
This looks good to me. I think I have my head around the main changes and they all seem sane. If it results in a significant speedup then that sounds good to me.
With the change in indentation for most of resolver.py resulting in a large diff, it's obviously a bit hard to check every change, so I may have missed some things, but I suspect the tests should catch almost everything.
There were just a couple of things I noticed on the way through, in separate comments, I don't think they should block this being merged.
| fluent_args = {} | ||
| if args is not None: | ||
| for argname, argvalue in args.items(): | ||
| fluent_args[argname] = native_to_fluent(argvalue) |
There was a problem hiding this comment.
nit: this looks like it could be written more succinctly using a dict comprehension
fluent_args = ({} if args is None else
{argname: native_to_fluent(argvalue) for argname, argvalue in args.items()})
There was a problem hiding this comment.
I've done half of this, as I found the inline if to be too hard to read. I made a top-level if, and then the dict comprehension or an empty dict.
| def compile(self, node): | ||
| nodename = type(node).__name__ | ||
| if not hasattr(resolver, nodename): | ||
| return node |
There was a problem hiding this comment.
With this kind of dynamic lookup of attributes on the resolver module, I think it would help comprehension if:
- The resolver module gained an
__all__attribute for show what it is exporting clearly, including all theBaseResolversubclasses. - Somewhere next to that there was a brief explanation of how these classes are looked up.
Otherwise someone new to the code base sees the resolver.Message class, for instance, but can't find any other references to it or work out how it is actually used.
There was a problem hiding this comment.
I added a doc string to the module and cleaned up the global namespace a bit more.
I didn't go for adding an __all__ after all. I started doing it, and then looked at the impact it had on our code paths, and there weren't that I could see. In particular, the pre-evaluator imports the module, and the complete namespace is on the module object. The only thing that would differ is from fluent.runtime.resolver import *, which we're not doing. Thus the __all__ would mostly be a source of typos and overlooked changes.
This changes a test, I don't think that the returned value should contain any non-source text fragments from a FluentNone.
I wonder if we should throw on bad external variables instead of making them a silent runtime error.
…the module is doing
|
Thanks for the review, I'll land the status quo. I'd be happy to see some follow-up issues filed for things that we can improve. |
This is my all-test-passing state of the proposal I made in https://discourse.mozilla.org/t/python-fluent-runtime-plans/35290/10.
I've given this a try to see how this is doing before we do changes to the non-compiling API that don't perform, notably I'm looking at #92.
This isn't yet in a state where it can be reviewed, sadly, because I didn't get to remove unused code yet. I wouldn't be surprised if this could end up being the same amount of code in the end as the current dispatch resolver.
The good news is that in the template benchmark, I cut down mean and median to 50% of what they're on master right now.