[batch] All public images must be under DOCKER_PREFIX/hailgenetics - #12230
Conversation
| f'{docker_prefix}/{name}' | ||
| for name in ('hailgenetics/hail', 'hailgenetics/genetics', 'python-dill', 'batch-worker') | ||
| ] | ||
| return [f'{docker_prefix}/hailgenetics/{name}' for name in ('hail', 'genetics', 'python-dill')] |
There was a problem hiding this comment.
We might need to special case the authentication / docker pull for the batch-worker image to make sure the JVM containers can use it.
There was a problem hiding this comment.
We already do
hail/batch/batch/worker/worker.py
Lines 432 to 433 in 25bafae
There was a problem hiding this comment.
Sorry one last thing. Do we need to add backwards compatibility for python-dill?
There was a problem hiding this comment.
This depends on whether or not you think what was there is a bug. I think it's a bug that we exposed gcr.io/hail-vdc to users. Currently, specifying j.image('hailgenetics/python-dill:3.7') doesn't work, but specifying j.image('gcr.io/hail-vdc/python-dill:3.7') does. It is the only instance where a user can explicitly access an image in gcr.io/hail-vdc. It seems really hacky to add in a compatibility layer that specifically rewrites gcr.io/hail-vdc/python-dill to {DOCKER_PREFIX}/hailgenetics/python-dill, especially considering that we're moving off of gcr.io/hail-vdc entirely.
There was a problem hiding this comment.
I feel like an announcement of a breaking bug fix is more appropriate, but if you think this is too much breakage then the other option is to do the rewrite.
There was a problem hiding this comment.
Let me double check with KC and the rest of our users to see how many people were actually using this.
There was a problem hiding this comment.
I imagine many were building their own images on top, in which case that is unaffected by this change
| f'{docker_prefix}/{name}' | ||
| for name in ('hailgenetics/hail', 'hailgenetics/genetics', 'python-dill', 'batch-worker') | ||
| ] | ||
| return [f'{docker_prefix}/hailgenetics/{name}' for name in ('hail', 'genetics', 'python-dill')] |
There was a problem hiding this comment.
Sorry one last thing. Do we need to add backwards compatibility for python-dill?
are we ok doing this now? Looks like there's no complaints in the zulip thread
|
Not quite sure why this was re-marked WIP. Anyway, switching to AR meant users lost access to |
|
I added the WIP because I thought from Zulip that it wasn't necessary to make the change for the artifact registry conversion and Dan thought it might be rude to do so, hence why I thought we might need to discuss it more. You can take the WIP tag off, but remind me to add this breaking change to the announcement to users about the changes for this week. |
3 participants
We should treat
python-dilllike other images in thehailgeneticsDockerHub repo and not hard-code our own registry into the docs. I also removed thebatch-workerimage from the publicly available images, not sure why that was in there but it seems wrong.