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 hugie config modify sub-command (resolves #41) #47

Merged
merged 9 commits into from
Jan 11, 2023

Conversation

ivyleavedtoadflax
Copy link
Contributor

@ivyleavedtoadflax ivyleavedtoadflax commented Nov 18, 2022

Description

Checklist

  • Tests added
  • Relevant issue mentioned
  • Documentation updated

@ivyleavedtoadflax ivyleavedtoadflax changed the title Feature/config editing Add hugie config modify sub-command Nov 26, 2022
@ivyleavedtoadflax ivyleavedtoadflax marked this pull request as ready for review November 27, 2022 00:02
@ivyleavedtoadflax ivyleavedtoadflax changed the title Add hugie config modify sub-command Add hugie config modify sub-command (resolves #41) Nov 27, 2022
@nsorros
Copy link
Contributor

nsorros commented Nov 28, 2022

Dont forget to tick the checklist

@ivyleavedtoadflax
Copy link
Contributor Author

@nsorros maybe you forgot to approve this?

│ list List all the deployed endpoints │
│ logs Get logs about an endpoint │
│ test Test an endpoint │
│ update Update an endpoint │
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this contain config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's a separate sub command, so it should be in docs/config

def test_inference_endpoint_config_serialization(data_path):
config = InferenceEndpointConfig.from_json(data_path)
config = dict(config)
assert isinstance(config, dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better to test that some data are still there instead of lost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 667d83a

@nsorros
Copy link
Contributor

nsorros commented Dec 9, 2022

i had envisioned this slightly differently and closer to

hugie configure --profile client-api-1

this would kick an interactive session where you can set some of the variables you define

    accountId: str = typer.Option(
        None, help="ID of the account (for private endpoints)"
    ),
    name: str = typer.Option(None, help="Name of the endpoint"),
    type: str = typer.Option(
        None, help="Type of endpoint, one of ['public', 'protected', 'private']"
    ),
    accelerator: str = typer.Option(
        "None", help="Accelerator to use. One of ['CPU','GPU']"
    ),
    instanceType: str = typer.Option(None),
    instanceSize: str = typer.Option(None),
    minReplica: int = typer.Option(None, help="Minimum number of replicas"),
    maxReplica: int = typer.Option(None, help="Maximum number of replicas"),
    framework: str = typer.Option("huggingface", help="Framework to use"),
    image: str = typer.Option(
        None,
        help="Image to use when deploying model endppint. Must be string representing a valid JSON, e.g. '{'huggingface': {}}'",
    ),
    repository: str = typer.Option(None, help="Name of the hf model repository"),
    revision: str = typer.Option(None, help="Revision of the hf model repository"),
    task: str = typer.Option(None, help="Task of the model"),
    vendor: str = typer.Option(None, help="Vendor to use. One of ['aws','gcp']"),
    region: str = typer.Option(None, help="Vendor specific region, e.g. 'us-east-1'"),

the end goal being that if a user has these variables setup or if they have them as env variables (maybe for the future) then they can simply do hugie endpoint create.

maybe a middle ground is to have hugie config modify be more interactive? so at least the user is creating a config file interactively which they can use for hugie endpoint create instead of passing params. it should also work with a path that does not exist and create it.

Modify an existing endpoint config file
"""

config = InferenceEndpointConfig.from_json(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work with a path that does not exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should fail. I will add tests and error handling for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error handling already covered by load_json(). In order to create the file (if it doesn't exist), we would need to have all the other arguments set, otherwise the file would not be properly specified. I would leave this for the current PR and let's discuss about the next one in relation to #54.

Noe that I was also considering adding a hugie config create to create a skeleton config file. Perhaps these two will be related.

@nsorros
Copy link
Contributor

nsorros commented Dec 9, 2022

I was also thinking that hugie configure should allow you to set those variables in a default profile and when the user tries to run hugie endpoint create they can leverage the default profile for most of those and only pass the information they want to change

@ivyleavedtoadflax
Copy link
Contributor Author

ivyleavedtoadflax commented Dec 9, 2022

I was also thinking that hugie configure should allow you to set those variables in a default profile and when the user tries to run hugie endpoint create they can leverage the default profile for most of those and only pass the information they want to change

I don't think a default profile will be very useful for a production setting. We want to be explicitly setting those things and recording them in Git, not relying on a hidden state that exists on someone's laptop.

This implements the second point you mention where you can specify an existing config file and make small changes to it on the command line making it easier to deploy and update deployments in CI/CD. The overwrite option allows you to save the new config out so that it can be stored in Git. We are already doing something like this in a Github action for one repo, so whether or not we add additional profile based functionality, I think we should merge hugie config modify so that that functionality is included in hugie, and doesn't need to be replicated elsewhere

@ivyleavedtoadflax
Copy link
Contributor Author

ivyleavedtoadflax commented Dec 9, 2022

i had envisioned this slightly differently and closer to

hugie configure --profile client-api-1

this would kick an interactive session where you can set some of the variables you define

    accountId: str = typer.Option(
        None, help="ID of the account (for private endpoints)"
    ),
    name: str = typer.Option(None, help="Name of the endpoint"),
    type: str = typer.Option(
        None, help="Type of endpoint, one of ['public', 'protected', 'private']"
    ),
    accelerator: str = typer.Option(
        "None", help="Accelerator to use. One of ['CPU','GPU']"
    ),
    instanceType: str = typer.Option(None),
    instanceSize: str = typer.Option(None),
    minReplica: int = typer.Option(None, help="Minimum number of replicas"),
    maxReplica: int = typer.Option(None, help="Maximum number of replicas"),
    framework: str = typer.Option("huggingface", help="Framework to use"),
    image: str = typer.Option(
        None,
        help="Image to use when deploying model endppint. Must be string representing a valid JSON, e.g. '{'huggingface': {}}'",
    ),
    repository: str = typer.Option(None, help="Name of the hf model repository"),
    revision: str = typer.Option(None, help="Revision of the hf model repository"),
    task: str = typer.Option(None, help="Task of the model"),
    vendor: str = typer.Option(None, help="Vendor to use. One of ['aws','gcp']"),
    region: str = typer.Option(None, help="Vendor specific region, e.g. 'us-east-1'"),

the end goal being that if a user has these variables setup or if they have them as env variables (maybe for the future) then they can simply do hugie endpoint create.

maybe a middle ground is to have hugie config modify be more interactive? so at least the user is creating a config file interactively which they can use for hugie endpoint create instead of passing params. it should also work with a path that does not exist and create it.

See my comment below on why I think that we need to rely on config files. We can add an interactive mode to config modify for the middle ground, but I think setting up profiles is the wrong way to go. The only corollary to this is that it might make sense to set some values in a common config file, such a cloud provider, etc., but this shouldn't be hidden in a local profile.

@nsorros
Copy link
Contributor

nsorros commented Dec 9, 2022

See my comment below on why I think that we need to rely on config files. We can add an interactive mode to config modify for the middle ground, but I think setting up profiles is the wrong way to go. The only corollary to this is that it might make sense to set some values in a common config file, such a cloud provider, etc., but this shouldn't be hidden in a local profile.

Setting an interactive mode might be a nice user experience, add an issue if you agree and we can implement in a later PR

@ivyleavedtoadflax
Copy link
Contributor Author

See my comment below on why I think that we need to rely on config files. We can add an interactive mode to config modify for the middle ground, but I think setting up profiles is the wrong way to go. The only corollary to this is that it might make sense to set some values in a common config file, such a cloud provider, etc., but this shouldn't be hidden in a local profile.

Setting an interactive mode might be a nice user experience, add an issue if you agree and we can implement in a later PR

Agree, I'll add an issue 👍

@ivyleavedtoadflax
Copy link
Contributor Author

Also bumping to 0.3.0 to cover new functionality.

@ivyleavedtoadflax ivyleavedtoadflax merged commit 93be962 into main Jan 11, 2023
@ivyleavedtoadflax ivyleavedtoadflax deleted the feature/config-editing branch January 11, 2023 10:16
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.

Add funcionality for config editing
2 participants