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

LUMI-127 adding inference API #328

Merged
merged 21 commits into from
Nov 14, 2024
Merged

LUMI-127 adding inference API #328

merged 21 commits into from
Nov 14, 2024

Conversation

aittalam
Copy link
Member

@aittalam aittalam commented Oct 30, 2024

What's changing

Added support for inference in our API:

  • new schema for input parameters and update job route
  • created ad-hoc inference configuration template and updated job service code to populate it
    Updated inference job to support the new configuration:
  • defined an InferenceJobConfig pydantic class to validate input dictionary
  • updated inference code to make use of it
  • added unit tests to validate minimal / full configurations

How to test it

  • unit tests on backend and SDK with make test
  • unit tests on inference job: cd lumigator/python/mzai/jobs/inference; uv run --with pytest --with-requirements requirements.txt --isolated pytest
  • manual test: test_dataset_upload && test_inference_mistral

test_dataset_upload:

#!/bin/bash
if [ "$#" -gt 0 ]; then
    DATA_CSV_PATH="$1"
else
    DATA_CSV_PATH="$HOME/Downloads/thunderbird_gt_bart.csv"
fi

if [[ -z "${BACKEND_URL}" ]]; then
  BACKEND_URL=http://localhost:8000
fi

echo Connecting to $BACKEND_URL...

curl -s $BACKEND_URL/api/v1/datasets/ \
  -H 'Accept: application/json' \
  -H 'Content-Type: multipart/form-data' \
  -F 'dataset=@'"$DATA_CSV_PATH"';type=text/csv' \
  -F 'format=job' | jq

test_inference_mistral:

#!/bin/bash

if [[ -z "${BACKEND_URL}" ]]; then
  BACKEND_URL=http://localhost:8000
fi

DATASET_ID=$(curl -s $BACKEND_URL/api/v1/datasets/ | jq -r '.items |sort_by(.created_at) | reverse | .[0].id')

EVAL_NAME="test_run_inference"
EVAL_DESC="Test run for inference with Mistral"
EVAL_MODEL="mistral://open-mistral-7b"
EVAL_DATASET=$DATASET_ID
EVAL_MAX_SAMPLES="0"

JSON_STRING=$(jq -n \
                --arg name "$EVAL_NAME" \
                --arg desc "$EVAL_DESC" \
                --arg model "$EVAL_MODEL" \
                --arg dataset_id "$EVAL_DATASET" \
                --arg max_samples "$EVAL_MAX_SAMPLES" \
                '{name: $name, description: $desc, model: $model, dataset: $dataset_id, max_samples: $max_samples}' )

echo Connecting to $BACKEND_URL...

curl -s $BACKEND_URL/api/v1/jobs/inference/ \
  -H 'Accept: application/json' \
  -H 'Content-Type: application/json' \
  -d "$JSON_STRING" | jq

Additional notes for reviewers

I already...

  • added some tests for any new functionality
  • updated the documentation: NO (the current job did not exist before and I wanted to complete the sequence of PRs before having full documentation for this, please LMK if that's ok otherwise I am happy to add some)
  • checked if a (backend) DB migration step was required and included it if required: no DB migration step was required (this PR does not change the tables schema)

@aittalam
Copy link
Member Author

@javiermtorres I added you as a reviewer because I saw the SDK integration test failed, but I had no issues testing locally with make test. lmk how I can help fix that (or do schemas just need to be rebuilt?)

@javiermtorres
Copy link
Contributor

@aittalam I will include the SDK integration tests in the test target (maybe they're not already included).

The issue is the split of JobCreate, so it is not found by the integration tests. Let me provide a branch that can be merged into yours to solve this issue.

@veekaybee
Copy link
Member

For reference, complementary job in progress: #333

@aittalam aittalam changed the title LUMI-127 adding inference aPI LUMI-127 adding inference API Nov 1, 2024
Copy link
Contributor

@ividal ividal left a comment

Choose a reason for hiding this comment

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

Thanks, @aittalam !

I'd be happy to ship this and start using it if we can remove part of the dupe code create_inference and create_eval have 😄 Just cleaning up one corner at a time.

.vscode/settings.json Show resolved Hide resolved
lumigator/python/mzai/backend/backend/services/jobs.py Outdated Show resolved Hide resolved
aittalam and others added 8 commits November 2, 2024 01:16
Includes SDK code, and mocked and integration tests.

Co-authored-by: Javier Torres <[email protected]>
* Support Alembic in tests

---------

Signed-off-by: Peter Wilson <[email protected]>
* Changed local development favoring docker-compose watch&sync instead of volume mounting to avoid .venv conflicts
---------

Signed-off-by: Alejandro Gonzalez <[email protected]>
Co-authored-by: Alejandro Gonzalez <[email protected]>
* Update README.md

Signed-off-by: Nate Brake <[email protected]>

* Update README.md

Ooh good idea, thanks!

Co-authored-by: Vicki Boykis <[email protected]>
Signed-off-by: Nate Brake <[email protected]>

---------

Signed-off-by: Nate Brake <[email protected]>
Co-authored-by: Vicki Boykis <[email protected]>
@aittalam
Copy link
Member Author

Ok all, here are my updates:

  • addressed reviews
  • rebased / merged code with current main
  • re-tested with pytest
  • re-tested with local tests specifically on the inference job
  • re-tested with API calls by manually submitting new inference jobs (which btw are not captured by integration tests yet, I'll add an issue for the next PR)

LMK if anything else should be done or if this is good to go

@veekaybee
Copy link
Member

LGTM after those few changes, thanks for all your patience on this! 🚀

@aittalam aittalam merged commit f9adf40 into main Nov 14, 2024
6 checks passed
@aittalam aittalam deleted the davide/LUMI-127-inference-api branch November 14, 2024 17:58
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.

7 participants