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

Refactor Configs #218

Merged
merged 25 commits into from
Sep 22, 2023
Merged

Refactor Configs #218

merged 25 commits into from
Sep 22, 2023

Conversation

mrwyattii
Copy link
Contributor

@mrwyattii mrwyattii commented Aug 4, 2023

In order to better support multi-model deployments and associated features from #208 we have identified some key ways in which the configs can be refactored. This will be the first of 2-3 PRs that does some refactor work and implements smaller segments of the work done in #208 to make review and merging easier.

@TosinSeg

Work done in this PR:

  • Refactor our configs to have MIIConfig and DeploymentConfig. This is in anticipation of having multi-model deployments where we will be passing in multiple DeploymentConfig objects
    • This could probably be extended in this PR to also include the DeepSpeed InferenceConfig rather than recreating that config in load_models.py (see comments I left on the code changes)
  • With the refactoring of our configs, we can simplify a lot of our code:
    • Avoid packing/unpacking config data at various stages of the deployment. Instead, just pass around the mii_config object as it contains everything we want. This massively simplifies the score template and server class
    • Move lots of parameter and config verification that was scattered around the codebase into the config classes themselves. This consolidates code to a common location and simplifies the process of debugging config settings
  • Light formatting changes. We use yapf for formatting, but this allows mixed ' and " - I don't think we need to use one or the other, but being consistent is good. Applied black and then yapf to get some nicer formatting (especially on the long assert lines)
    • Is it too late to switch the project to use black? @jeffra

I left lots of comments about changes I felt unsure about or where I could use some feedback, however the PR is mostly ready to merge. We could always handle minor cleanup and changes in future PRs that implement the rest of #208

@mrwyattii mrwyattii marked this pull request as ready for review August 8, 2023 23:10
Copy link
Contributor

@tohtana tohtana left a comment

Choose a reason for hiding this comment

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

Thank you for this great effort! I've noticed that the areas I had concerns about have been improved. This will make future development much more pleasant.

@mrwyattii mrwyattii merged commit 44026a6 into main Sep 22, 2023
@mrwyattii mrwyattii deleted the mrwyattii/refactor-config branch September 22, 2023 22:45
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.

3 participants