ClassyCat in Presto: CV2 4789 #97
Conversation
ashkankzme
requested review from
DGaffney,
computermacgyver and
skyemeedan
as code owners
test/lib/model/test_classycat.py
Outdated
| } | ||
| ] | ||
| }, | ||
| "callback_url": "http://host.docker.internal:9888" |
There was a problem hiding this comment.
It looks like tests are failing at an earlier stage right now around getting CLASSYCAT_BATCH_SIZE_LIMIT, but I suspect the tests will also fail with a timeout from http://host.docker.internal:9888 on Linux. This has been my experience.
If you are using Docker-for-mac or Docker-for-Windows 18.03+, connect to your mysql service using the host host.docker.internal (instead of the 127.0.0.1 in your connection string).
If you are using Docker-for-Linux 20.10.0+, you can also use the host host.docker.internal if you started your Docker container with the --add-host host.docker.internal:host-gateway option, or added the following snippet in your docker-compose.yml file :
extra_hosts:
- "host.docker.internal:host-gateway"
There was a problem hiding this comment.
thanks for flagging Scott.
I think the tests failing bc of CLASSYCAT_BATCH_SIZE_LIMIT (or other en vars) is due to the fact that these new environment variables are not being correctly set in the test cases. I will look into what the issue is and try to figure it out.
re: callbacks not working -- I think that makes sense and is expected. the address mentioned there is a local server I have spun up to catch the callbacks from Presto. I did not commit the server with the PR code (but happy to do so if helpful), and therefore it doesn't exist to receive the results.
I'm now realizing that the other tests have been mocking the callback functionality, and I have not done so for classycat. I will do that soon and update the PR. it should fix the timeout error.
There was a problem hiding this comment.
Yes, my personal preference as well is mocking callback functionality for intra-container tests
There was a problem hiding this comment.
Looks great, awesome how compact it is!
- I'm still not clear how a consumer of the model will get the schemas uploaded and deployed (in the tests it is assuming that consumer has a local copy of the schema text to submit before doing classification)
- does it need some docker-compose.py entries to be able to spin up model locally?
- I think it needs more examples and validation of input and output data structures to document what needs to be submitted
lib/s3.py
Outdated
| # Extract the file name from the local file path | ||
| try: | ||
| s3_client.head_bucket(Bucket=bucket) | ||
| except ClientError as e: | ||
| if e.response['Error']['Code'] == 'NoSuchBucket' or int(e.response['Error']['Code']) in [403, 404]: | ||
| # Create the bucket since it does not exist | ||
| s3_client.create_bucket(Bucket=bucket) | ||
| logger.info(f'Created bucket {bucket} in MinIO.') | ||
| logger.info(f'Created bucket {bucket} in MinIO.') # is this accurate? i.e. is this code only for minio? |
There was a problem hiding this comment.
I guess in S3 the assumption is the bucket already exists?
There was a problem hiding this comment.
this is code that I only refactored, and it had existed prior to classycat.
I think what could change here is the logging message: depending on which environment we are operating in, we could be making buckets in either minio or s3. the code is not just limited to running on minio only.
you could assume that buckets already exist in s3 but I think in case the assumption is not true, this may cause confusion in case this code is run on production.
not a big deal though
There was a problem hiding this comment.
I believe the default pattern we established was to create the bucket locally but not in deployed environments, and that the create_bucket would fail because of permissions errors. Tagging @sonoransun to confirm/deny
There was a problem hiding this comment.
I just updated the log message to be a bit clearer (feel free to push back) -- it now says creating a bucket in s3 (or minio, if running locally)
.env_file.example
Outdated
| CACHE_DEFAULT_TTL=86400 | ||
|
|
||
| CLASSYCAT_OUTPUT_BUCKET="litterbox-dev" |
There was a problem hiding this comment.
lol I've been "why is the output bucket called 'litterbox'? oh" but seems a bit cryptic when looking at a list of s3 buckets. would classycat-qa, classycat-live work?
There was a problem hiding this comment.
haha, sure we can change the bucket naming
There was a problem hiding this comment.
+1 for keeping it litterbox, lol
| elif event_type == 'schema_lookup' or event_type == 'schema_create': | ||
| result_instance = ClassyCatSchemaResponse(**result_data) | ||
| else: | ||
| result_instance = ClassyCatResponse(**result_data) | ||
| elif 'video' in model_name: | ||
| result_instance = VideoResponse(**result_data) | ||
| else: |
There was a problem hiding this comment.
I think this else should be elif should raise error if unknown model name (otherwise will silently fail on typo?)
There was a problem hiding this comment.
I don't think it will fail silently as is, since we check for the event type when processing the request later. please look at classycat.py/process().
I did this so I can respond with the same error structure as other classycat request, otherwise you may not receive an error directly from presto or it may be in a different format.
There was a problem hiding this comment.
oh I was wrong. I understand now this is just mesage validation, I thought it was parsing incoming messages. I lost a couple of hours debugging because I was sending messages with the model 'yake' instead of 'yake_keywords' and it just silently accepted ... but this isn't the place to enforce that.
There was a problem hiding this comment.
hm, for what its worth @skyemeedan we could raise an error if the model name is unknown to this function...
There was a problem hiding this comment.
@DGaffney we do respond with an error to the caller in this case, it's just handled in the process function instead of here. happy to discuss further!
There was a problem hiding this comment.
Just to clarify, @ashkankzme , I believe Devin was referring to a Presto model (YAKE, Video, Audio, etc.) and I believe you're referring to an LLM model. I think that's a good idea to respond with an error if the Presto model is unknown. That, however, can be a separate ticket / PR
There was a problem hiding this comment.
oh I see what you mean, my bad. I do think this should be a separate ticket since we would want to test all/different presto models before making such changes. I will create a ticket for it.
There was a problem hiding this comment.
here is the ticket: https://meedan.atlassian.net/browse/CV2-4986
lib/model/classycat_classify.py
Outdated
| max_tokens=(max_tokens_per_item * items_count) + 15, | ||
| temperature=0.5 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Can we check here for some of the OpenRouter specific errors (like out of tokens) so the model can error instead of passing downstream?
There was a problem hiding this comment.
trying to better understand the scope, can you please explain a bit more which cases this would cover, and what the end goal would be?
There was a problem hiding this comment.
Currently timpani sees errors like 'not enough credits', but doesn't know what that means, so it is going to keep sending additional requests. I think if classcat is unable to do any processing, it should be reporting that as an exception, and perhaps crashing/going offline to signal for help?
There was a problem hiding this comment.
I see. I agree that it is helpful for the consumer of this service to distinguish between when it is ok to retry upon errors vs when it's a matter of service downtime and it's not ok to retry. I'll try to incorporate this into the code!
There was a problem hiding this comment.
Let's defer this until the PR for consistent error/status output and error handling is in place (this is in separate work)
adding a comment for validating responses generated from LLMs (just the ticket) Co-authored-by: Skye Bender-deMoll <[email protected]>
# Conflicts: # docker-compose.yml
… in other tests, the callbacks are not being mocked, e.g. test_image.py, and tests run fine locally
There was a problem hiding this comment.
Looks great. Let's get this merged and on QA. I want to make sure we do a full test of video similarity on QA given this touches S3. There's also a few follow-up tickets we should put in the backlog (e.g., Presto should respond with an error if the Presto model requested is unknown, logging metrics for ClassyCat, etc.)---can you please file those?
| elif event_type == 'schema_lookup' or event_type == 'schema_create': | ||
| result_instance = ClassyCatSchemaResponse(**result_data) | ||
| else: | ||
| result_instance = ClassyCatResponse(**result_data) | ||
| elif 'video' in model_name: | ||
| result_instance = VideoResponse(**result_data) | ||
| else: |
There was a problem hiding this comment.
Just to clarify, @ashkankzme , I believe Devin was referring to a Presto model (YAKE, Video, Audio, etc.) and I believe you're referring to an LLM model. I think that's a good idea to respond with an error if the Presto model is unknown. That, however, can be a separate ticket / PR
adding logging to mark where future tickets will be implemented Co-authored-by: Skye Bender-deMoll <[email protected]>
Description
This PR is for implementing the three main classycat endpoints into presto: schema_create, schema_lookup, and classify
Reference: TICKET-ID (to provide additional context)
https://meedan.atlassian.net/jira/software/c/projects/CV2/issues/CV2-4789
How has this been tested?
Has it been tested locally? Yes
Are there automated tests? Yes, unit tests under test_classycat.py
Are there any external dependencies?
Are there changes required in sysops terraform for this feature or fix?
Have you considered secure coding practices when writing this code?
Please list any security concerns that may be relevant.
N/A