fix: Convert Pydantic model to dict for run_model_container#7
fix: Convert Pydantic model to dict for run_model_container#7lawrencelai wants to merge 1 commit intomainfrom
Conversation
…s dict. I found auto_deployed wrongly pass the Object (DeployResources). To fix this, I convert the object to dict and pass it into the method
phoevos
left a comment
There was a problem hiding this comment.
Hey @lawrencelai, thanks for looking into this.
Please take a look at my comment and make sure that all tests pass.
Apart from that, note that we require signed commits for merging to main; please look into configuring git with your GPG key and using it to sign your commits before pushing to GH: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
| deployment_type=ModelDeploymentType.AUTO, | ||
| ttl=model_config.idle_ttl, | ||
| resources=model_config.deploy.resources if model_config.deploy else None, | ||
| resources=model_config.deploy.__dict__ if model_config.deploy else None, |
There was a problem hiding this comment.
Any reason for not using pydantic's built-in model_dump here?
We generally tend to avoid calling internal/magic/dunder methods like that directly. __dict__ is used to fetch an object's writable attributes, not convert it to a dictionary. Consider the following example:
deploy = DeploySpec(resources=DeployResources(limits=ResourceLimits(memory="4g")))Calling deploy.__dict__ will return the following, which isn't really what you want (notice how it doesn't work recursively):
{'resources': DeployResources(limits=ResourceLimits(memory='4g', cpus=None), reservations=None)}Instead, you should be calling model_dump which is build for this exact purpose, replacing the now deprecated dict method of pydantic's BaseModel.
| resources=model_config.deploy.__dict__ if model_config.deploy else None, | |
| resources=model_config.deploy.model_dump() if model_config.deploy else None, |
The above will give you the intended result:
{'resources': {'limits': {'memory': '4g', 'cpus': None}, 'reservations': None}}|
I would also recommend updating the PR title to something more descriptive of the actual change/fix rather than a vague reference to the error that exposed the bug! Following the conventions used in this repo, I would go for something along the lines of |
core.model.run_model_container method, parameter "resources" define as dict. I found auto_deployed.py wrongly pass the Object (DeployResources). To fix this, I convert the object to dict and pass it into the method