Make Funsor compatible with Python 3.5 #329
Conversation
|
bye bye f-strings 😢 |
|
This is going to be a significant review effort because we have started using the ordered dictionary assumption, but Python 3.5 doesn't follow ordered dictionary semantics. I think we'll need to replace |
I think it's mostly confined to the tests and perhaps to |
There was a problem hiding this comment.
Looks great. Only minor comments.
Could we spend an hour or so poking around code and discussing issues to convince each other that all determinism issues are resolved? We can do that after this PR merges. The goal of this PR is unusual in that it is difficult to verify by reviewing a diff on github; rather I'd like to search the full Funsor codebase for possible sources of nondeterminism.
| OrderedDict(), | ||
| OrderedDict([('i', bint(2))]), | ||
| OrderedDict([('i', bint(2)), ('j', bint(3))]), | ||
| ], ids=id_from_inputs) | ||
| @pytest.mark.parametrize('real_inputs', [ | ||
| {'x': reals()}, | ||
| {'x': reals(4)}, | ||
| {'x': reals(2, 3)}, | ||
| {'x': reals(), 'y': reals()}, | ||
| {'x': reals(2), 'y': reals(3)}, | ||
| {'x': reals(4), 'y': reals(2, 3), 'z': reals()}, | ||
| OrderedDict([('x', reals())]), | ||
| OrderedDict([('x', reals(4))]), | ||
| OrderedDict([('x', reals(2, 3))]), | ||
| OrderedDict([('x', reals()), ('y', reals())]), | ||
| OrderedDict([('x', reals(2)), ('y', reals(3))]), | ||
| OrderedDict([('x', reals(4)), ('y', reals(2, 3)), ('z', reals())]), |
There was a problem hiding this comment.
nit: I think we can revert these changes. I had set this test up so that we could write concise {}-dicts in the param statements, then convert them to OrderedDict via OrderedDict(list(sorted(xxx.items()))) below. I think this is still nice to do for readability.
Please let me know if I've misunderstand some issue with pytest collection and the OrderedDict change is required.
There was a problem hiding this comment.
The OrderedDict changes to test parameters in this PR are required for using pytest-xdist. Otherwise, test collection will fail with multiple workers.
There was a problem hiding this comment.
Thanks for explaining! I'll avoid this pattern in the future.
| @@ -45,6 +45,11 @@ | |||
| np.seterr(all='ignore') | |||
|
|
|||
|
|
|||
| # have to make this deterministic for pytest collection to work | |||
There was a problem hiding this comment.
Could you explain the type of error you saw, so I can keep an eye out for similar errors in the future?
There was a problem hiding this comment.
pytest-xdist seems to require all workers to have identical test cases that appear in the same order. Some test cases in this file are generated by looping over the keys of REDUCE_OP_TO_NUMERIC, which is a dictionary. If you revert this change and attempt to run the tests in Python 3.5 with multiple workers (pytest -n auto), test collection will nondeterministically fail because workers may not agree.
| if objects != types and supercedes(types, objects): | ||
| register(*objects)(self.default) | ||
| except TypeError: | ||
| pass # mysterious source of ambiguity in Python 3.5 breaks this |
There was a problem hiding this comment.
This is the only significant change in this PR. It places stricter constraints on method resolution order in multiple dispatch in cases that could be ambiguous, and otherwise has no effect. If this is reverted, attempting to run the test suite in Python 3.5 will lead to a bunch of strange, nondeterministic and difficult-to-diagnose test failures ultimately caused by patterns not firing when they ought to.
There was a problem hiding this comment.
Weird. Do you know whether the Type error occurs in !=, supercedes(), and, or register()()?
There was a problem hiding this comment.
Adding supercedes here fixes the test failures, but introduces occasional TypeErrors which can be disregarded.
| if objects != types and supercedes(types, objects): | ||
| register(*objects)(self.default) | ||
| except TypeError: | ||
| pass # mysterious source of ambiguity in Python 3.5 breaks this |
There was a problem hiding this comment.
Weird. Do you know whether the Type error occurs in !=, supercedes(), and, or register()()?
| OrderedDict(), | ||
| OrderedDict([('i', bint(2))]), | ||
| OrderedDict([('i', bint(2)), ('j', bint(3))]), | ||
| ], ids=id_from_inputs) | ||
| @pytest.mark.parametrize('real_inputs', [ | ||
| {'x': reals()}, | ||
| {'x': reals(4)}, | ||
| {'x': reals(2, 3)}, | ||
| {'x': reals(), 'y': reals()}, | ||
| {'x': reals(2), 'y': reals(3)}, | ||
| {'x': reals(4), 'y': reals(2, 3), 'z': reals()}, | ||
| OrderedDict([('x', reals())]), | ||
| OrderedDict([('x', reals(4))]), | ||
| OrderedDict([('x', reals(2, 3))]), | ||
| OrderedDict([('x', reals()), ('y', reals())]), | ||
| OrderedDict([('x', reals(2)), ('y', reals(3))]), | ||
| OrderedDict([('x', reals(4)), ('y', reals(2, 3)), ('z', reals())]), |
There was a problem hiding this comment.
Thanks for explaining! I'll avoid this pattern in the future.
Necessary for use with Pyro, which supports Python 3.5 and uses it in Travis.
As far as I can tell, this PR contains all of the changes necessary to support Python 3.5 - I couldn't find any lurking order dependence in the obvious places, especially the distribution wrappers. The only substantive issue I found and fixed was caused by a mysterious nondeterministic edge case in
multipledispatch, perhaps related to #243. Otherwise, most of the changes are either removals of f-strings or replacements of dictionaries withOrderedDicts in test case lists to make pytest test collection deterministic.