Skip to content
This repository has been archived by the owner on Jun 25, 2023. It is now read-only.

Initial commit #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Initial commit #28

wants to merge 4 commits into from

Conversation

krokozyab
Copy link

Hello, please test it.

Copy link
Collaborator

@rsolovev rsolovev left a comment

Choose a reason for hiding this comment

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

Hey @krokozyab thank you for the interest in this challenge and for your solution.

As you can see, our gh actions failed to build your solution, we suspect the base image being too large (i.e python:3.11). Could you please try using pytorch/pytorch:*-cuda*-cudnn*-runtime image (we generally use those for this purpose) or if you want the base to be pure python, python:3.11-slim should do the trick. Thank you in advance!

Copy link
Collaborator

@rsolovev rsolovev left a comment

Choose a reason for hiding this comment

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

Hey @krokozyab, thank you for the fix, now tests ran as expected. Few notes:

  1. Here are our tests results on a grafana dashboard. Errors you see are HTTP-400 errors returned from empty texts (these are intended and need to be handled as valid input)
  2. You forgot to remove FROM python:3.11-slim from the Dockerfile and runtime CUDA was missing (docker "ignored" first FROM). Commenting out 3rd line fixes this issue

Once again than you for this beautiful solution and if you want you can improve or optimise it and re-requests review. Please address aforementioned issues if you plan to do so. Every contribution will count while choosing a challenge winner.

2. Fixed dockerfile.
3. Introduced fastapi workers.
Copy link
Collaborator

@rsolovev rsolovev left a comment

Choose a reason for hiding this comment

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

@krokozyab thank you for this additions, here are tests results

and here is one example of a text that returns HTTP-500 I was able to catch:

"🇧🇫http://www.directsynergistic.org/reinvent/transform/visionary. \x1e\x18\x1e$j}3[ 3G6puXYZHcJ3ZlflmKUYTH1hNv3YCEw👣 You can't validate the microchip without calculating the redundant CSS array!"

@krokozyab
Copy link
Author

Hi, I guess (only guess) the test does not simulate multisession, and all request sticks to just one worker so the multi-worker solution just makes the solution worse.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants