python-cachier / cachier

Persistent, stale-free, local and cross-machine caching for Python functions.
MIT License
545 stars 63 forks source link

Handle function defaults correctly #42

Closed filyp closed 3 years ago

filyp commented 3 years ago

Right now, if you call some with function without specifying a default parameter, cache remembers it as 'parameter not set'. If you change this default value in function definition, and call again without specifying this parameter, cache will incorrectly return the function output for the old value. Also, if you sometimes use the default and sometimes specify the value, the function will be be computed two times, needlessly.

It can be easily fixed by replacing this code: https://github.com/shaypal5/cachier/blob/5b0bff9af3dbd251ae657e4f6f27f269de977768/cachier/pickle_core.py#L152 with:

bound = inspect.signature(function).bind(*args, **kwargs)
bound.apply_defaults()
key = tuple((k, value) for k, value in sorted(bound.arguments.items()))

If you want this improvement I can do a PR.

shaypal5 commented 3 years ago

Definitely! I would love a PR! Just don't forget to add a test case fo which the old code fails!

filyp commented 3 years ago

I started by running the tests (without making any changes). As README says, I ran:

pip install -e ".[test]"
python -m pytest --cov=cachier

And got:

ImportError while loading conftest '/home/filip/projects/cachier/tests/conftest.py'.
tests/conftest.py:6: in <module>
    from .test_mongo_core import _test_mongetter
tests/test_mongo_core.py:62: in <module>
    ???
cachier/core.py:159: in cachier
    core = _MongoCore(mongetter, stale_after, next_time, wait_for_calc_timeout)
cachier/mongo_core.py:47: in __init__
    self.mongo_collection = self.mongetter()
tests/test_mongo_core.py:47: in _test_mongetter
    _test_mongetter.client = _get_cachier_db_mongo_client()
tests/test_mongo_core.py:33: in _get_cachier_db_mongo_client
    client.cachier_test.authenticate(
../../../.local/lib/python3.9/site-packages/pymongo/database.py:1492: in authenticate
    self.client._cache_credentials(
../../../.local/lib/python3.9/site-packages/pymongo/mongo_client.py:775: in _cache_credentials
    server = self._get_topology().select_server(
../../../.local/lib/python3.9/site-packages/pymongo/topology.py:241: in select_server
    return random.choice(self.select_servers(selector,
../../../.local/lib/python3.9/site-packages/pymongo/topology.py:199: in select_servers
    server_descriptions = self._select_servers_loop(
../../../.local/lib/python3.9/site-packages/pymongo/topology.py:215: in _select_servers_loop
    raise ServerSelectionTimeoutError(
E   pymongo.errors.ServerSelectionTimeoutError: ds119508.mlab.com:19508: [Errno -2] Name or service not known, Timeout: 30s, Topology Description: <TopologyDescription id: 6026ba66fb8b4f290eeb013e, topology_type: Single, servers: [<ServerDescription ('ds119508.mlab.com', 19508) server_type: Unknown, rtt: None, error=AutoReconnect('ds119508.mlab.com:19508: [Errno -2] Name or service not known')>]>

Is it an issue with ds119508.mlab.com or do I need to set something up?

shaypal5 commented 3 years ago

No no. It's a problem on my end.

The mlab server I used to run tests is down. I now have to either come up with another free solution (shouldn't be hard), or host a local server for each tests or something. IDK.

shaypal5 commented 3 years ago

Hey @fsondej !

Ok, I've set up an alternative free server on Atlas mongodb.com. All tests are now passing. You will only be able to test MongoDb core functionalities when opening a PR from your fork, as the credentials are set up on travis, and it will run all tests on a PR.

You can now continue with your good work! :)

filyp commented 3 years ago

Ok, I opened a draft PR #50