add a coerce_collections key_factory for @memoized to reduce boilerplate #7127

cosmicexplorer · 2019-01-23T04:14:16Z

Problem

I accidentally left a TODO in a previous PR to allow @memoized_method to coerce lists into tuples instead of erroring out (because lists aren't hashable), which I left again in #7126.

It would be nice to not make things tuples everywhere (weird and unclear syntax, imho) just so we can memoize them.

Solution

Introduce coercing_arg_normalizer to apply a dict of type coercions to arguments for a key_factory.
Introduce coerce_collections{,_per_instance} as key_factorys for @memoized to coerce list and OrderedSet inputs to tuples so they can be hashed.
Add testing.

Result

Some boilerplate is removed when interacting with functions or instance methods decorated with forms of @memoized.

benjyw · 2019-01-23T05:05:36Z

I think @jsirois knows the most about this code.

cosmicexplorer · 2019-01-23T05:06:41Z

Thanks, added! I definitely thought I had clicked that but I think a lot of things.

cosmicexplorer · 2019-01-23T07:40:06Z

The only CI failures are building linux and osx wheels, which are also failing in other PRs, so this is reviewable (and I will look into why this is happening, although perhaps tomorrow).

jsirois

It would be nice to actually see one real result as described in this PR - is there one instance of boilerplate that can be removed?

jsirois · 2019-01-23T15:31:09Z

tests/python/pants_test/util/test_memo.py

+    self.assertEqual(6, g(OrderedSet(x)))
+    self.assertEqual(2, g_called._calls)
+
+    class C(self._Called):


This should break out into its own test method, its testing a different function.

jsirois · 2019-01-23T15:32:38Z

tests/python/pants_test/util/test_memo.py

+      def g(self, x):
+        self._called()
+        return sum(x)
+    c = C()


The key_factory is _per_instance but you only ever make one instance. It would be good to make two and confirm each instance carries its own independent cache.

jsirois · 2019-01-23T15:37:07Z

src/python/pants/util/memo.py

+  return tuple(coerced)
+
+
+collection_coercions = {


Maybe _COLLECTION_COERCIONS - this serves in the private constant role IIUC.

jsirois · 2019-01-23T15:42:15Z

src/python/pants/util/memo.py

@@ -24,6 +26,44 @@ def equal_args(*args, **kwargs):
  return key


+def coercing_arg_normalizer(type_coercions, base_normalizer, args, kwargs):


Seems this can be _private, fwict you only intend to export coerce_collections and coerce_collections_per_instance.

jsirois · 2019-01-23T15:46:39Z

src/python/pants/util/memo.py

+
+  :rtype: tuple
+  """
+  args_key = base_normalizer(*args, **kwargs)


There is no contract whatsoever on what the cache key that is output is shaped like. It could be a cryptographic-grade hash value. Normalization should really apply to *args and **kwargs and the transformed versions of those be passed to the base_normalizer. As things stand you're presenting a pretend api that actually only works with equal_args et. al. One option is to not pretend this is a generic api. You could take a bool flag instead of base_normalizer, say per_instance. If True - pick per_instance here and otherwise use equal_args. By reparamterizing in that way you could leave the code here as you have it and couple to the knowledge of the output shape of those key factories.

jsirois · 2019-01-23T15:48:52Z

src/python/pants/util/memo.py

@@ -24,6 +26,44 @@ def equal_args(*args, **kwargs):
  return key


+def coercing_arg_normalizer(type_coercions, base_normalizer, args, kwargs):


It would be wonderful to keep consistent terminology in a single file. base_normalizer is elsewhere key_factory. Maybe coercing_key_factory and base_key_factory or underlying or else rename everything related in this file to *normalizer.

cosmicexplorer · 2019-01-24T20:21:21Z

is there one instance of boilerplate that can be removed?

Not in this PR (but I will look for one), but in #7126: there's a TODO that mentions this issue, and the boilerplate would occur here:

pants/tests/python/pants_test/backend/python/tasks/util/build_local_dists_test_base.py

Lines 139 to 141 in b2175b7

    
           run_before_synthesized_task_types = self._synthesize_task_types(tuple(self._run_before_task_types)) 
        
           python_create_distributions_task_type = self._testing_task_type 
        
           run_after_synthesized_task_types = self._synthesize_task_types(tuple(self._run_after_task_types))

After having refactored that code since it was first written, the @classpropertys could be declared as tuples, as we do with e.g. subsystem_dependencies() (in a previous iteration of that PR that was not the case, and the memoization was needed for correctness because we were calling _synthesize_task_types() multiple times in the same method, motivating this separate change). The rationale for this change is that the collection type (tuple) required for subclasses to implement e.g. _run_before_task_types takes on an arbitrary importance due to the implementation detail of memoization, which is confusing (this is much more the reason than "weird syntax").

I will address your inline comments and look for another example of boilerplate that may make this more convincing today.

Eric-Arellano · 2020-05-17T22:32:06Z

Closing due to being stale and review feedback that this would benefit from a use case being included in the PR. Feel free to reopen, of course, if you still have a need for the feature.

cosmicexplorer added 2 commits January 22, 2019 20:01

add coerce_collections{,_per_instance}() key_factorys for @memoized

a6dfb94

remove now-outdated TODO!

670527b

cosmicexplorer requested review from stuhood and benjyw January 23, 2019 04:14

cosmicexplorer requested a review from jsirois January 23, 2019 05:05

jsirois reviewed Jan 23, 2019

View reviewed changes

cosmicexplorer mentioned this pull request Jan 28, 2019

add a wrapper_type argument to TypedCollection #7172

Closed

Eric-Arellano closed this May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a coerce_collections key_factory for @memoized to reduce boilerplate #7127

add a coerce_collections key_factory for @memoized to reduce boilerplate #7127

cosmicexplorer commented Jan 23, 2019

benjyw commented Jan 23, 2019

cosmicexplorer commented Jan 23, 2019

cosmicexplorer commented Jan 23, 2019 •

edited

jsirois left a comment

jsirois Jan 23, 2019

jsirois Jan 23, 2019

jsirois Jan 23, 2019

jsirois Jan 23, 2019

jsirois Jan 23, 2019

jsirois Jan 23, 2019

cosmicexplorer commented Jan 24, 2019 •

edited

Eric-Arellano commented May 17, 2020

		@@ -24,6 +26,44 @@ def equal_args(args, *kwargs):
		return key


		def coercing_arg_normalizer(type_coercions, base_normalizer, args, kwargs):

add a coerce_collections key_factory for @memoized to reduce boilerplate #7127

add a coerce_collections key_factory for @memoized to reduce boilerplate #7127

Conversation

cosmicexplorer commented Jan 23, 2019

Problem

Solution

Result

benjyw commented Jan 23, 2019

cosmicexplorer commented Jan 23, 2019

cosmicexplorer commented Jan 23, 2019 • edited

jsirois left a comment

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

jsirois Jan 23, 2019

Choose a reason for hiding this comment

cosmicexplorer commented Jan 24, 2019 • edited

Eric-Arellano commented May 17, 2020

cosmicexplorer commented Jan 23, 2019 •

edited

cosmicexplorer commented Jan 24, 2019 •

edited