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

Dependency injection for model deployments #1787

Closed
wants to merge 4 commits into from

Conversation

yifanmai
Copy link
Collaborator

This allows using ModelDeploymentRegistry with clients that do not have a api_key parameter.

Example:

helm-run --run-specs boolq:model=simple/my-simple-model --suite debug -m 5 --model-deployment-paths model_deployments_simple.yaml --model-metadata models.yaml

models.yaml:

models:
  - name: simple/my-simple-model

model_deployments_simple.yaml:

model_deployments:
  - name: simple/my-simple-model
    model_name: simple/my-simple-model
    tokenizer_name: "huggingface/gpt2"
    max_sequence_length: 2048
    client_spec:
      class_name: "helm.proxy.clients.simple_client.SimpleClient"
      args: {}

@yifanmai yifanmai changed the title Yifanmai/fix auto client injection Dependency injection for model deployments Aug 16, 2023


def create_object(spec: ObjectSpec, additional_args: Optional[Dict[str, Any]] = None):
"""Create the actual object given the `spec`."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Elaborate on documentation of additional_args

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...I think it might be cleaner / more modular to have a function that takes a spec and updates the spec with additional_args, so that the create_object function can stay the same.

args = {}
args.update(spec.args)
missing_args = []
for parameter_name in init_signature.parameters.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm confused what injectors are supposed to do...

@yifanmai
Copy link
Collaborator Author

Closing because this is addressed by #1861.

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.

2 participants