Conversation
| # For async postgres, we need to use an async dialect. | ||
| if not sync: | ||
| engine_url = engine_url.replace("postgresql://", "postgresql+asyncpg://") | ||
| engine_url = engine_url.replace("postgresql://", "postgresql+asyncpg://").replace( |
There was a problem hiding this comment.
what does replace('sslmode', 'ssl') do? does it remain compatible with our aws db setup?
There was a problem hiding this comment.
For sync, psycopg2 needs sslmode, but for async, asyncpg needs ssl. This shouldn't affect AWS because the sslmode=require param is only added for Azure
| return f"rediss://{username}:{password}@{self.cache_redis_azure_host}" | ||
|
|
||
| @property | ||
| def cache_redis_url_expiration(self) -> Optional[int]: |
There was a problem hiding this comment.
I originally thought this was something representing a "timedelta", should we call it cache_redis_url_expiration_timestamp? (or otherwise make it clear it represents a timestamp/point in time)
| latest_tag = self.docker_repository.get_latest_image_tag( | ||
| hmi_config.batch_inference_vllm_repository | ||
| ) | ||
| except ResourceNotFoundError: |
There was a problem hiding this comment.
If we wanted to be strict about clean architecture, I feel like we'd probably want to have the service not need to know about azure (which means having the docker repository bubble up an error that was not specific to AWS/Azure/etc., and have the docker repositories themselves catch the cloud-specific error and raise the non-cloud-specific error).
Also, would we want to catch any errors that AWS/boto3 might throw at us as well?
There was a problem hiding this comment.
Ohhh yeah, I forgot this could/should be in the Docker repository instead 😅 Will fix! 🙂 Might want to do something similar for AWS if the vllm repo doesn't exist, but feels out of scope for this PR 😛
seanshi-scale
left a comment
seanshi-scale
left a comment
There was a problem hiding this comment.
had a few more comments, but they're pretty minor. LGTM once they're addressed
| return await use_case.execute( | ||
| user=auth, | ||
| filename=file.filename, | ||
| filename=file.filename or "", |
There was a problem hiding this comment.
when does file.filename equal None, should we just throw a 4xx in that case (if it is user fault that file.filename could be None?)
maybe this is fine if filename doesn't get used as the only part of an identifier actually, tbh I'm not sure though
There was a problem hiding this comment.
Hmm not sure, I'm assuming this came up from lint because file.filename changed from str to Optional[str] between the old and new FastAPI versions... kinda hoping it's just always defined lol 😅
There was a problem hiding this comment.
do we know when filename can be None? eg does the fastapi documentation say anything about it
There was a problem hiding this comment.
Hmm I couldn't find anything in the documentation, but it does seem like there are methods of calling where this can be user-set
| image = client.list_manifest_properties( | ||
| repository_name, order_by="time_desc", results_per_page=1 | ||
| ).next() | ||
| return image.tags[0] |
There was a problem hiding this comment.
do we want to throw an error if there are 0 image tags?
There was a problem hiding this comment.
unless that's already handled in the ResourceNotFoundError
There was a problem hiding this comment.
Just tested, it looks like Azure automatically deletes repositories that are empty, so it'll be a ResourceNotFoundError 🤔
There was a problem hiding this comment.
ah ok sounds good, could we note it in the code so we know why we're not gonna IndexError?
3 participants
Pull Request Summary
What is this PR changing? Why is this change being made? Any caveats you'd like to highlight? Link any relevant documents, links, or screenshots here if applicable.
Test Plan and Usage Guide
How did you validate that your PR works correctly? How do you run or demo the code? Provide enough detail so a reviewer can reasonably reproduce the testing procedure. Paste example command line invocations if applicable.