Implement image pull policies #233
Conversation
|
@tillahoffmann This is ready for review. Do you mind taking a look? |
There was a problem hiding this comment.
Looks good, I've left some inline comments.
One larger question is whether we need the additional complexity of the image cache. Directly getting the images from the client takes about 5 ms on my machine. That seems negligible compared with typical runtimes of tests because of the container start-up time. Removing the cache could simplify the code a little. What do you think?
| @@ -1,5 +1,5 @@ | |||
| # | |||
| # This file is autogenerated by pip-compile with python 3.6 | |||
| # This file is autogenerated by pip-compile with python 3.10 | |||
There was a problem hiding this comment.
This should probably be regenerated with 3.6.
There was a problem hiding this comment.
I'm getting:
pip-compile --output-file=requirements/3.6.txt requirements.in
ERROR: Could not find a version that satisfies the requirement python-arango==7.4.1 (from testcontainers===dev->-r requirements.in (line 1)) (from versions: 3.0.0, 3.1.0, 3.2.0, 3.2.2, 3.3.0, 3.4.0, 3.4.1, 3.5.0, 3.6.0, 3.7.0, 3.8.0, 3.9.0, 3.10.0, 3.10.1, 3.11.0, 3.12.1, 4.0.0, 4.0.1, 4.1.0, 4.2.0, 4.2.1, 4.3.0, 4.4.0, 5.0.0, 5.1.0, 5.2.0, 5.2.1, 5.3.0, 5.4.0, 6.0.0, 6.1.0, 7.0.0, 7.0.1, 7.1.0, 7.2.0, 7.3.0, 7.3.1)
Traceback (most recent call last):
File "/Users/okatz6/.pyenv/versions/3.6.15/bin/pip-compile", line 11, in <module>
sys.exit(cli())
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/click/core.py", line 1128, in __call__
return self.main(*args, **kwargs)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/click/core.py", line 1053, in main
rv = self.invoke(ctx)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/click/core.py", line 1395, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/click/core.py", line 754, in invoke
return __callback(*args, **kwargs)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/click/decorators.py", line 26, in new_func
return f(get_current_context(), *args, **kwargs)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/scripts/compile.py", line 466, in cli
results = resolver.resolve(max_rounds=max_rounds)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/resolver.py", line 175, in resolve
has_changed, best_matches = self._resolve_one_round()
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/resolver.py", line 319, in _resolve_one_round
their_constraints.extend(self._iter_dependencies(best_match))
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/resolver.py", line 428, in _iter_dependencies
dependencies = self.repository.get_dependencies(ireq)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/repositories/local.py", line 79, in get_dependencies
return self.repository.get_dependencies(ireq)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/repositories/pypi.py", line 237, in get_dependencies
download_dir, ireq, wheel_cache
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/piptools/repositories/pypi.py", line 199, in resolve_reqs
results = resolver._resolve_one(reqset, ireq)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/pip/_internal/resolution/legacy/resolver.py", line 379, in _resolve_one
dist = self._get_dist_for(req_to_install)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/pip/_internal/resolution/legacy/resolver.py", line 331, in _get_dist_for
self._populate_link(req)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/pip/_internal/resolution/legacy/resolver.py", line 300, in _populate_link
req.link = self._find_requirement_link(req)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/pip/_internal/resolution/legacy/resolver.py", line 266, in _find_requirement_link
best_candidate = self.finder.find_requirement(req, upgrade)
File "/Users/okatz6/.pyenv/versions/3.6.15/lib/python3.6/site-packages/pip/_internal/index/package_finder.py", line 910, in find_requirement
"No matching distribution found for {}".format(req)
pip._internal.exceptions.DistributionNotFound: No matching distribution found for python-arango==7.4.1 (from testcontainers===dev->-r requirements.in (line 1))
Any idea why?
There was a problem hiding this comment.
@tillahoffmann ping
There was a problem hiding this comment.
This should do the trick.
testcontainers-python/Makefile
Lines 14 to 21 in b65c8a0
If not, let's drop 3.6 as you suggested in another issue.
| only if it does not exist locally | ||
| """ | ||
|
|
||
| def should_pull_cached(self, image: Image) -> bool: |
There was a problem hiding this comment.
This is a no-op, right? We could just write DefaultPullPolicy = ImagePullPolicy or drop one of them.
There was a problem hiding this comment.
Because ImagePullPolicy is abstract.
There was a problem hiding this comment.
My hunch is that we can remove the notion of an abstract ImagePullPolicy because python uses duck-typing (as opposed to more interface-heavy Java, for example). Some discussion here.
|
|
||
|
|
||
| def test_default_pull_policy_cached_image(): | ||
| ImagePullPolicy.IMAGES_CACHE = MagicMock(spec=_LocalImagesCache) |
There was a problem hiding this comment.
Can we use with unittest.mock.patch here to make sure the original values are restored after the test is done?
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
+ Coverage 86.32% 87.12% +0.79%
==========================================
Files 27 33 +6
Lines 746 823 +77
Branches 70 81 +11
==========================================
+ Hits 644 717 +73
- Misses 79 82 +3
- Partials 23 24 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I ported the Java code to work and the cache is present there, which is why it is present here. Note that the same container might be stopped or started multiple times and 5ms for a single query regarding its age might add up to a lot in a very large test suite. |
|
I have a relatively strong preference to remove the cache for two reasons. First, it's more code to maintain. Second, I don't think the caching will help in most real-world scenarios. I appreciate that runtime is different across different machines, but getting a single image takes about 4ms on my machine. Using the cache, getting a single image takes about 300ms. It'll of course be quicker the next time images are fetched, but we'd have to use at least 75 containers per test suite to make the cache worthwhile. To have a delay of one second over the cached version, we'd have to run about 325 containers. I'd argue that for a test suite that runs more than 300 docker containers a delay of one second is negligible. But let me know if I misunderstood/miscalculated. |
These policies exist in testcontainers-java and are currently missing from this project.
They allow you to specify when you should pull a new image.