Skip to content
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

Add support for submitting a prebuilt image #8802

Merged
merged 38 commits into from
May 27, 2024

Conversation

kiendang
Copy link
Contributor

@kiendang kiendang commented May 13, 2024

  • Rename submit_dockerfile to submit_container_image.
  • submit_container_image now accepts a docker_config of a more generic WorkerConfig type which could be DockerWorkerConfig or PrebuiltWorkerConfig
  • Add unit tests

Works with (modified) notebook 11. Only need to

  • Add integration test

Rename submit_dockerfile to submit_container_image

submit_container_image now accepts a docker_config of a more generic WorkerConfig type which could be DockerWorkerConfig or PrebuiltWorkerConfig

Add unit tests
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

kiendang and others added 2 commits May 13, 2024 16:21
ruff has already been configured to format notebook files as well.
@shubham3121
Copy link
Member

We might also have to generalize the get_by_docker_config stash method to get both Prebuilt and DockerConfig

kiendang added 5 commits May 14, 2024 14:47
get_by_worker_config now accepts a config of a more generic WorkerConfig type which could be DockerWorkerConfig or PrebuiltWorkerConfig
in services.worker_image.submit_container_image and
services.worker_image.get_by_config
@kiendang kiendang force-pushed the prebuilt-image-worker branch from 8fe7089 to 16bcf91 Compare May 16, 2024 11:13
@kiendang kiendang force-pushed the prebuilt-image-worker branch from 1c99dd4 to f9ec9d3 Compare May 16, 2024 12:01
@kiendang kiendang force-pushed the prebuilt-image-worker branch from f9ec9d3 to 749bc1e Compare May 16, 2024 12:36
@kiendang kiendang force-pushed the prebuilt-image-worker branch from d4bb721 to f2c4d5c Compare May 16, 2024 14:00
@kiendang kiendang force-pushed the prebuilt-image-worker branch from be68096 to b0b658a Compare May 16, 2024 14:33
@kiendang kiendang force-pushed the prebuilt-image-worker branch from 1e283b4 to b9554e5 Compare May 16, 2024 15:52
@kiendang kiendang force-pushed the prebuilt-image-worker branch 2 times, most recently from 5e3a8f3 to b2a4ba4 Compare May 21, 2024 17:19
@kiendang kiendang force-pushed the prebuilt-image-worker branch from b2a4ba4 to f9a35e1 Compare May 21, 2024 17:58
@kiendang
Copy link
Contributor Author

kiendang commented May 22, 2024

@yashgorana @shubham3121 May I use this opportunity to correct some inconsistencies in arg names of various API endpoints?

  • worker_image_service.push(image: UID =) where everywhere else uses image_uid
  • worker_image_service.build(pull: bool =), worker_pool_service.create_image_and_pool_request(pull_image: bool =)
  • worker_pool_service.launch(name: str = ) where everywhere else uses pool_name
  • reg in reg_username and reg_password is very ambiguous. We should change to registry_username and registry_password. They're not much longer and we already use registry_uid.

@kiendang
Copy link
Contributor Author

We also use tag in a lot of places where it should be image name instead. python:3-slim is not the image tag, it's the image name. 3-slim is the tag.

@madhavajay
Copy link
Collaborator

We also use tag in a lot of places where it should be image name instead. python:3-slim is not the image tag, it's the image name. 3-slim is the tag.

Yes although, a tag by itself and an image by itself arent really useful, we can extract the image and repo from the tag, but not the other way around.

I would call this image_tag or something where image and tag are mandatory but but repo is optional.

Copy link
Member

@shubham3121 shubham3121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @kiendang !! 🎉 💯
Great work generalising the APIs for submitting worker configs.

@kiendang
Copy link
Contributor Author

kiendang commented May 24, 2024

@yashgorana @shubham3121 May I use this opportunity to correct some inconsistencies in arg names of various API endpoints?

* `worker_image_service.push(image: UID =)` where everywhere else uses `image_uid`

* `worker_image_service.build(pull: bool =)`, `worker_pool_service.create_image_and_pool_request(pull_image: bool =)`

* `worker_pool_service.launch(name: str = )` where everywhere else uses `pool_name`

* `reg` in `reg_username` and `reg_password` is very ambiguous. We should change to `registry_username` and `registry_password`. They're not much longer and we already use `registry_uid`.

Will put these in a separate PR.

@@ -110,7 +110,7 @@ def test_create_pool_request_accept(
assert root_client.credentials != ds_client.credentials

# the DO submits the docker config to build an image
submit_result = root_client.api.services.worker_image.submit_container_image(
submit_result = root_client.api.services.worker_image.submit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

@shubham3121 shubham3121 enabled auto-merge May 27, 2024 08:27
@shubham3121 shubham3121 merged commit 54cad0a into OpenMined:dev May 27, 2024
23 checks passed
@kiendang kiendang deleted the prebuilt-image-worker branch May 27, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants