feat: support offline executors #2935
Conversation
- default to disk cache when fetching hubble executor meta information if there's no internet connection
- skip pulling the docker image if there's no internet connection and the docker image was already pulled
|
This PR closes: #120 |
Codecov Report
@@ Coverage Diff @@
## master #2935 +/- ##
==========================================
+ Coverage 88.85% 88.99% +0.13%
==========================================
Files 138 138
Lines 9578 9607 +29
==========================================
+ Hits 8511 8550 +39
+ Misses 1067 1057 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
a37a734 to
d9cba05
Compare
tests/unit/hubble/test_helper.py
Outdated
| def test_disk_cache(): | ||
| @disk_cache((Exception,)) | ||
| def test_disk_cache(tmpdir): | ||
| global first_time |
There was a problem hiding this comment.
what is first_time needed? Isn't there a better way? globals are scary
There was a problem hiding this comment.
disk_cache works this way:
- if exception is raised but no entry in cache => raise exception
- if exception is raised but there's an entry in cache exists => use it
- if no exception is raised, store the result in cache
So I wanted to simulate all 3 cases, first by raising an exception when the cache is empty (first_time=False)
then by having a successful call and storing result in cache (first_time=True) and finally raising an exception, but this time we find a result in a cache so we use it (first_time=True)
I can't use it as a function parameter because it would change the cache key.
Do you have another idea ?
There was a problem hiding this comment.
but u do not need a global. U can have a local variable inside the test and manipulate it there
There was a problem hiding this comment.
Can't access it : UnboundLocalError: local variable 'first_time' referenced before assignment
There was a problem hiding this comment.
never mind I know now why that shows up
jina/hubble/helper.py
Outdated
| def disk_cache( | ||
| exceptions: Iterable[Exception], | ||
| cache_file: str = 'disk_cache.db', | ||
| message: str = "Calling {func_name} failed, using cached results", |
jina/hubble/helper.py
Outdated
| @@ -250,3 +251,40 @@ def upload_file( | |||
| response = getattr(requests, method)(url, data=data, headers=headers, stream=stream) | |||
|
|
|||
| return response | |||
|
|
|||
|
|
|||
| def disk_cache( | |||
There was a problem hiding this comment.
Let's do this one more explicit and do not let add Exceptions as one wants.
Handling exceptions as normal behavior is a little dangerous and while it makes sense for this case, I would prefer to discourage it and specialize better this decorator
There was a problem hiding this comment.
So this means hardcoding the exception right ?
Should I rename the decorator ?
tests/unit/hubble/test_helper.py
Outdated
| def _myfunc() -> bool: | ||
| global first_time | ||
| if not first_time: | ||
| raise _Exception("Failing") |
ca35980 to
19dc7e9
Compare
tests/unit/hubble/test_helper.py
Outdated
| def test_disk_cache(): | ||
| @disk_cache((Exception,)) | ||
| def test_disk_cache(tmpdir): | ||
| global first_time |
There was a problem hiding this comment.
but u do not need a global. U can have a local variable inside the test and manipulate it there
jina/hubble/hubio.py
Outdated
| self._client.images.get(executor.image_name) | ||
| except docker.errors.ImageNotFound: | ||
| img_not_found = True | ||
| if img_not_found: |
There was a problem hiding this comment.
this seems weird, why not just raising and avoid this extra boolean?
There was a problem hiding this comment.
Actually I need to re-raise APIError.
Maybe I can create a helper function docker_image_exists(image_name) because the same logic is also used in other places (dockerize.py, container/__init__.py).
I'm worried about the usage of docker (if I'll put it in a helper function) since docker is not installed in all tags.
What do you think ?
There was a problem hiding this comment.
anyway, u can do except APIError as apiex, and then raise apiex. And u would not need this variable
198d444 to
0ee09a5
Compare
jina/hubble/hubio.py
Outdated
| self._client.images.get(executor.image_name) | ||
| except docker.errors.ImageNotFound: | ||
| img_not_found = True | ||
| if img_not_found: |
There was a problem hiding this comment.
anyway, u can do except APIError as apiex, and then raise apiex. And u would not need this variable
0b030dd to
2ec8fb9
Compare
2ec8fb9 to
11dc2f0
Compare
|
Thanks for your contribution ❤️ Note, other CI tests will not start until the commit messages get fixed. This message will be deleted automatically when the commit messages get fixed. |