[RFC] Provide an explicit Cache API #891

fpagnoux · 2019-06-21T07:13:01Z

Connected to #887

PR Content

This PR is WIP as it only defines some methods contracts rather than implementing them. Only one method is pseudo-implemented, to better explicit the separation of tasks.

Some unit tests are also added, to explicit the behavior of the introduces methods.

The intent is to reach a decision on the big picture before getting drowned into implementation details.

Approach

The suggestion here is to:

Dismantle and remove the Holder class
- Caching calculated variables would be moved to a new class Cache
- The rest of the logic would be moved to other existing classes
Make simulation.set_input the user-facing method to set and input for a variable at a given time

fpagnoux · 2019-06-21T07:27:27Z

openfisca_core/cache.py

+from openfisca_core.periods import Period
+
+
+class Cache:


This class interface looks very similar to the current InMemoryStorage. Maybe these classes could be merged ?

fpagnoux · 2019-06-21T07:29:46Z

openfisca_core/cache.py

+class Cache:
+
+
+    def get(self, variable: str, period: Period) -> Optional[Array]:


I initially thought about names longer like get_cached_array or put_in_cache, but as this class is just doing cache, it seemed redondant.

fpagnoux · 2019-06-21T07:31:43Z

openfisca_core/simulations.py

+        if period is not None:
+            period = periods.period(period)
+
+        if variable.is_inactive(period):


This replaces the check on variable.end

Avoid negation in method name and use is_active(period) instead?

fpagnoux · 2019-06-21T07:32:17Z

openfisca_core/simulations.py

            return
-        self.get_holder(variable_name).set_input(period, value)
+        array = variable.cast_to_array(value)


This replaces the _to_array method of Holder

Shorten the name to to_array() and add some documentation to the method as the user might wonder why an openfisca variable is involved?

fpagnoux · 2019-06-21T07:36:39Z

openfisca_core/simulations.py

-        self.get_holder(variable_name).set_input(period, value)
+        array = variable.cast_to_array(value)
+
+        if len(array) != population.count:


I was tempted to abstract this check into a method somewhere else, but the error message is pretty specific to the set_input context, and needs access both the variable and population.

fpagnoux · 2019-06-21T07:38:18Z

openfisca_core/simulations.py

+                )
+
+        if variable.set_input:
+            return variable.set_input(self, variable, period, array)


The set_input hooks need access to both simulation and variable. It could for sure be done better, but I'd rather do the minimum adaptation and keep further improvement out of scope as there have been recent work on that by @Morendil.

fpagnoux · 2019-06-21T07:39:00Z

openfisca_core/simulations.py

+        if variable.set_input:
+            return variable.set_input(self, variable, period, array)
+
+        variable.check_input_period(period)


This replaces the period checked that were spread on holder.set_input and holder._set

As there is no mention of the input in this method, it seems more general. Rename to check_period/is_valid_period/check_period_matching... and adapt documentation+log message accordingly?

fpagnoux · 2019-06-21T07:48:28Z

tests/core/test_u_cache.py

+
+
+@fixture
+def cache():


Methodology question for @Morendil :

Let's assume that Cache uses, in its implementation, a bunch of InMemoryStorage instances to store data. This is Cache internal business, and is not reflected in the public methods.

To write a proper unit test on Cache, do I need to stub/mock InMemoryStorage, or can I just write tests on the Cache methods, as below?

In other words, can a unit test indirectly run some code that is outside of the class under test, or does it make it an integration test ?

Thanks a lot in advance!

I try not to get too hung up on unit/integration test differences, it's a tactical matter rather than one of principle. It's fine if the methods of the object under test rely on some other collaborators, as long as you're primarily testing that object's behaviour, rather than indirectly testing the collaborators.

Ok, thanks a lot for the explanation 🙂

Morendil · 2019-06-22T10:51:36Z

openfisca_core/cache.py

+        """
+        pass
+
+    def get_known_periods(self, variable: str) -> List[Period]:


I'm not convinced this method needs to be part of the public contract. Who needs it? Under what circumstances?

Not sure about the exact circumstances, but it is definitely used in several places in IPP code, when they want to figure out all what's in the cache.

The cache is sometimes updated to init a calculation in a reform.
We might need a clear interface for reforms and, in the meantime, use get_known_periods, delete...

sandcha · 2019-07-09T15:44:18Z

openfisca_core/simulations.py

+        if period is not None:
+            period = periods.period(period)
+
+        if variable.is_inactive(period):


Avoid negation in method name and use is_active(period) instead?

sandcha · 2019-07-09T15:47:28Z

openfisca_core/simulations.py

            return
-        self.get_holder(variable_name).set_input(period, value)
+        array = variable.cast_to_array(value)


Shorten the name to to_array() and add some documentation to the method as the user might wonder why an openfisca variable is involved?

sandcha · 2019-07-09T15:55:23Z

openfisca_core/simulations.py

+        if variable.set_input:
+            return variable.set_input(self, variable, period, array)
+
+        variable.check_input_period(period)


As there is no mention of the input in this method, it seems more general. Rename to check_period/is_valid_period/check_period_matching... and adapt documentation+log message accordingly?

sandcha · 2019-07-09T16:02:21Z

tests/core/test_u_cache.py

+
+
+def test_does_not_retrieve(cache):
+    value = cache.get_cached_array('toto', period)


Suggested change

value = cache.get_cached_array('toto', period)

value = cache.get('toto', period)

sandcha · 2019-07-09T16:02:39Z

openfisca_core/simulations.py

+            return variable.set_input(self, variable, period, array)
+
+        variable.check_input_period(period)
+        self.cache.put_in_cache(variable.name, period, array)


Suggested change

self.cache.put_in_cache(variable.name, period, array)

self.cache.put(variable.name, period, array)

bonjourmauko · 2021-04-10T09:09:24Z

Thanks again @fpagnoux for this contribution, which inspired greatly a proposal of my own on #947. I'm closing as this proposal stales a bit, although the issue itself remains open as, at some point, I agree we need to get rid of holders 😃

fpagnoux added 5 commits June 21, 2019 08:45

Explicit new cache api contract

f88df97

Add unit tests for cache

b380a85

Draft simulation.set_input implem

dd1d900

Define variable new methods contract

9b47278

Add unit tests for variable

820bb39

fpagnoux commented Jun 21, 2019

View reviewed changes

fixup

7e51d93

fpagnoux commented Jun 21, 2019

View reviewed changes

fpagnoux requested review from Morendil and sandcha June 21, 2019 07:48

fpagnoux assigned Morendil Jun 21, 2019

Morendil reviewed Jun 22, 2019

View reviewed changes

sandcha reviewed Jul 9, 2019

View reviewed changes

bonjourmauko added the kind:discovery Issue requires discovery: value, ux and tech label Jul 23, 2019

bonjourmauko changed the title ~~[WIP] Provide an explicit Cache API~~ Provide an explicit Cache API Dec 10, 2019

sandcha mentioned this pull request Jan 15, 2020

Corrige les modalités de calcul de l'âge openfisca/openfisca-france#1346

Open

7 tasks

bonjourmauko self-assigned this Jan 27, 2020

bonjourmauko mentioned this pull request Feb 8, 2020

[RFC] Provide an explicit caching API #947

Closed

12 tasks

bonjourmauko changed the title ~~Provide an explicit Cache API~~ [WIP] Provide an explicit Cache API Apr 1, 2021

bonjourmauko added the policy:rfc Request For Comments: chime in! label Apr 1, 2021

bonjourmauko changed the title ~~[WIP] Provide an explicit Cache API~~ [WIP/RFC] Provide an explicit Cache API Apr 1, 2021

bonjourmauko closed this Apr 10, 2021

bonjourmauko changed the title ~~[WIP/RFC] Provide an explicit Cache API~~ [RFC] Provide an explicit Cache API Apr 10, 2021

bonjourmauko mentioned this pull request Sep 29, 2021

Provide an explicit caching API (v2) #1052

Closed

bonjourmauko deleted the new-api-cache branch December 6, 2022 21:54

		class Cache:


		def get(self, variable: str, period: Period) -> Optional[Array]:



		def test_does_not_retrieve(cache):
		value = cache.get_cached_array('toto', period)

	value = cache.get_cached_array('toto', period)
	value = cache.get('toto', period)

	self.cache.put_in_cache(variable.name, period, array)
	self.cache.put(variable.name, period, array)

[RFC] Provide an explicit Cache API #891

[RFC] Provide an explicit Cache API #891

Uh oh!

Conversation

fpagnoux commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Content

Approach

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpagnoux Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandcha Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bonjourmauko commented Apr 10, 2021

Uh oh!

Uh oh!

fpagnoux commented Jun 21, 2019 •

edited

Loading

fpagnoux Jun 21, 2019 •

edited

Loading

sandcha Jul 9, 2019 •

edited

Loading