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

cli: Fix deployment CLI not working with deploy . -f config.yaml #4845

Closed
wants to merge 3 commits into from

Conversation

jianshen92
Copy link
Contributor

What does this PR address?

bentoml deploy . -f config.yaml ignores the config file passed in.

Fixes #(issue)

Before submitting:

@jianshen92 jianshen92 requested a review from a team as a code owner July 9, 2024 17:19
@jianshen92 jianshen92 requested review from frostming and FogDong and removed request for a team and frostming July 9, 2024 17:19
@@ -73,26 +73,25 @@ class DeploymentConfigParameters:
def verify(
self,
):
deploy_by_param = (
self.name
or self.bento
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing self.name here

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 removing self.name or self.bento?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant removing self.bento

Copy link
Member

Choose a reason for hiding this comment

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

Why we need to remove self.bento here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise it would be rejected by the check:

if (
            (self.config_dict and self.config_file)
            or (self.config_dict and deploy_by_param)
            or (self.config_file and deploy_by_param)
        ):
            raise BentoMLException(
                "Configure a deployment can only use one of the following: config_dict, config_file, or the other parameters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because bentoml deploy . will consider . as the bento and this check evaluates to true and then it will skip the file argument

or self.envs
or self.extras
deploy_by_param = any(
param is not None and len(param) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid you can't judge with this, given there are integer values (scaling_min and scaling_max)
Can we just filter None, meaning it is specified by the users

Copy link
Contributor Author

@jianshen92 jianshen92 Jul 10, 2024

Choose a reason for hiding this comment

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

Yeah initially that was the case, but the default for self.envs is a list so it will always evaluate to True if we dont check for it.

@jianshen92
Copy link
Contributor Author

Decided to throw error if user use both inline argument and file.
User should specify the name in the bentofile, i.e name: '.'

CC: @Sherlock113 to update documentation

@jianshen92 jianshen92 closed this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants