Skip to content
This repository has been archived by the owner on Feb 25, 2024. It is now read-only.

fix: using buildx for bentoctl build #165

Closed
wants to merge 10 commits into from
Closed

Conversation

jjmachan
Copy link
Contributor

Description

closes:

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #165 (af94f3f) into main (54ca196) will increase coverage by 0.13%.
The diff coverage is 31.25%.

@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   60.39%   60.52%   +0.13%     
==========================================
  Files          24       24              
  Lines        1121     1135      +14     
==========================================
+ Hits          677      687      +10     
- Misses        444      448       +4     
Flag Coverage Δ
unit-tests 60.52% <31.25%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bentoctl/deployment_config.py 66.66% <0.00%> (ø)
bentoctl/operator/operator.py 71.23% <0.00%> (-3.06%) ⬇️
bentoctl/docker_utils.py 23.07% <26.66%> (+3.07%) ⬆️
bentoctl/utils/temp_dir.py 78.78% <37.50%> (+2.78%) ⬆️
bentoctl/cli/__init__.py 84.78% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@yubozhao yubozhao requested a review from aarnphm June 2, 2022 15:55
Comment on lines 151 to 153
build_docker_image(
image_tag=local_docker_tag,
context_path=dockercontext_path,
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we just use the containerize function directly instead of mapping this the another function?

from bentoml.bentos import contanerize
#
containerize(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

prefer not to maintain two same codes in two different repo

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we generating the Dockerfile in the expected path for the containerize command? If we are, we should use your suggestion

Copy link
Contributor

@aarnphm aarnphm Jun 3, 2022

Choose a reason for hiding this comment

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

instead of this function, just use containerize from bentoml

since build_docker_image is literally the same as containerize

containerize(bento_tag, local_docker_tag)

thus no need to write unit test on bentoctl side, since we will have tests to cover this on bentoml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doen't the containerize command load the bentos from the bento store? build_docker_image is used to build from the path.

Copy link
Member

Choose a reason for hiding this comment

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

We should try always build from bento store, only use path internally

Copy link
Member

Choose a reason for hiding this comment

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

Do we still have any user facing API in bentoctl that’s working directly with a bento path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do use the bento store available to resolve the path to the bento given a bento tag the user provides. Then we move it to a temporary directory and do the build from there. We move the bento to a temporary directory to be able to add or remove files that are required to make it deployable in the cloud.

No, we don't have any user-facing API with bento path. The user only provides the bento tag, which bentoctl resolves and copies to a directory and passes that directory to the operator. The operator is then free to make any required changes.

Copy link
Contributor

@aarnphm aarnphm Jun 6, 2022

Choose a reason for hiding this comment

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

can we unify the concept of the deployable to a bento? Sounds to me that the deployable is the same as a bento, with the target bento be a package inside the deployable bento

We already supports include folders under bentofile.yaml. I propose for all of the operator we treat it like a bento, provide a bentofile.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is a good idea but would need a bit of work since bentoctl internally depends on the concept of the deployable. the 'deployable' is just a fancy bento only diff is that it is not stored in the bento store and is meant to be a throwaway.

Let's come back to this at a later time after a bit more discussion?

@yubozhao yubozhao marked this pull request as ready for review June 3, 2022 13:29
@aarnphm aarnphm added the On-hold This PR is on hold, don't merge label Jun 6, 2022
@aarnphm
Copy link
Contributor

aarnphm commented Jun 10, 2022

close this since file changes need to changes I have added on the other PR.

@aarnphm aarnphm closed this Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
On-hold This PR is on hold, don't merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants