New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: cache hub pull with build env #5061
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5061 +/- ##
==========================================
- Coverage 88.07% 88.07% -0.01%
==========================================
Files 115 115
Lines 9149 9165 +16
==========================================
+ Hits 8058 8072 +14
- Misses 1091 1093 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There is already a pr to fix it #5054 |
I see now. Would it be possible to always add a description to your PRs? Otherwise we won't know what your PR does, and in a past retro we also agreed not to review PRs without description. Thank you! |
Yes. of course, and i will write a description about it. thank you. very much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor changes
Co-authored-by: Nan Wang <nan.wang@jina.ai>
Co-authored-by: Nan Wang <nan.wang@jina.ai>
I think the following code should be used
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should usebuild_env = build_env_dict if len(list(build_env_dict.keys())) > 0 else None
not build_env = build_env_dict if build_env_dict else None
. if use build_env = build_env_dict if build_env_dict else None
it is always true
there was a bug where pulling an Executor with build env vars would not work, because it tried to cache the keys of these env vars, but
DictKeys
object can't be pickled. This PR converts these keys to a list, thus fixing the bug.The diffs are large, but that's only because of
black
.The only relevant line is
build_env=list(buildEnv.keys()) if buildEnv else [],
, line 715 inhubio.py
(and the added test).