Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

Use new vllm/llama-cpp backends for evaluate - #1539

Merged
mergify[bot] merged 2 commits into
instructlab:mainfrom
danmcp:evalandvllm
Jul 2, 2024
Merged

Use new vllm/llama-cpp backends for evaluate#1539
mergify[bot] merged 2 commits into
instructlab:mainfrom
danmcp:evalandvllm

Conversation

@danmcp

@danmcp danmcp commented Jun 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Followup to #1369

This PR is using the new serving backends and support the vllm and llama_cpp path. It replaces temporary code serving vllm directly.

Checklist:

  • [ x] Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • [x ] Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@mergify mergify Bot added ci-failure PR has at least one CI failure testing Relates to testing and removed ci-failure PR has at least one CI failure labels Jun 30, 2024
@danmcp danmcp changed the title Use new vllm backend Use new vllm backend for evaluate Jun 30, 2024
@danmcp danmcp changed the title Use new vllm backend for evaluate Use new vllm/llama-cpp backends for evaluate Jun 30, 2024
@danmcp
danmcp requested review from alimaredia and cdoern June 30, 2024 15:58
@nathan-weinberg
nathan-weinberg self-requested a review July 1, 2024 01:09
nathan-weinberg
nathan-weinberg previously approved these changes Jul 1, 2024
View reviewed changes

@nathan-weinberg nathan-weinberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would like @alimaredia and @cdoern to signoff but apart from one logging comment LGTM

Comment thread src/instructlab/model/evaluate.py
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 1, 2024
leseb
leseb suggested changes Jul 1, 2024
View reviewed changes
Comment thread src/instructlab/configuration.py Outdated
Comment thread src/instructlab/model/evaluate.py Outdated
Comment thread src/instructlab/model/evaluate.py Outdated
Comment thread src/instructlab/model/evaluate.py Outdated
Comment thread src/instructlab/model/evaluate.py Outdated
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 1, 2024
@danmcp

danmcp commented Jul 1, 2024

Copy link
Copy Markdown
Contributor Author

Will need to adjust to changes in #1531

nathan-weinberg
nathan-weinberg reviewed Jul 1, 2024
View reviewed changes
Comment thread src/instructlab/configuration.py Outdated
Comment thread src/instructlab/model/evaluate.py
Comment thread src/instructlab/model/evaluate.py
Comment thread src/instructlab/model/evaluate.py
alimaredia
alimaredia reviewed Jul 1, 2024
View reviewed changes
Comment thread src/instructlab/model/evaluate.py
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jul 1, 2024
@danmcp

danmcp commented Jul 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like pylint is catching a dep issue outside this code:

Error: src/instructlab/model/train.py:11:0: E0611: No name 'run_training' in module 'instructlab.training' (no-name-in-module)

@nathan-weinberg

nathan-weinberg commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Looks like pylint is catching a dep issue outside this code:

Error: src/instructlab/model/train.py:11:0: E0611: No name 'run_training' in module 'instructlab.training' (no-name-in-module)

I hit the same in #1544 and merged anyway since it's unrelated to us

@nathan-weinberg
nathan-weinberg requested a review from leseb July 1, 2024 18:34
nathan-weinberg
nathan-weinberg previously approved these changes Jul 1, 2024
View reviewed changes
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 1, 2024
@nathan-weinberg

nathan-weinberg commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

@danmcp a rebase should fix the lint issue

@cdoern

cdoern commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

so one idea I have for the config is it would look like the following:

chat:
  context: default
  greedy_mode: false
  logs_dir: data/chatlogs
  max_tokens: null
  model: models/merlinite-7b-lab-Q4_K_M.gguf
  session: null
  vi_mode: false
  visible_overflow: true
evaluate:
  base_branch: null
  base_model: instructlab/granite-7b-lab
  branch: null
  mmlu:
    batch_size: 5
    few_shots: 2
  mmlu_branch:
    sdg_path: generated
  model: models/merlinite-7b-lab-Q4_K_M.gguf
  mt_bench:
    judge_model: prometheus-eval/prometheus-8x7b-v2.0
    max_workers: 40
    output_dir: eval_data
  mt_bench_branch:
    taxonomy_path: taxonomy
general:
  log_level: INFO
generate:
  chunk_word_count: 1000
  model: models/merlinite-7b-lab-Q4_K_M.gguf
  num_cpus: 10
  num_instructions: 100
  output_dir: generated
  prompt_file: prompt.txt
  seed_file: seed_tasks.json
  taxonomy_base: origin/main
  taxonomy_path: taxonomy
serve:
  backend: ''
  host_port: 127.0.0.1:8000
  llama_cpp:
    gpu_layers: -1
    llm_family: ''
    max_ctx_size: 4096
  model_path: models/merlinite-7b-lab-Q4_K_M.gguf
  vllm:
    vllm_args: []
train:
  torch_args:
    nnodes: 1
    node_rank: 0
    nproc_per_node: 1
    rdzv_endpoint: 127.0.0.1:12222
    rdzv_id: 123
  train_args:
    chat_tmpl_path: /home/cdoern/.pyenv/versions/3.11.9/lib/python3.11/site-packages/instructlab/training/chat_templates/ibm_generic_tmpl.py
    ckpt_output_dir: checkpoints
    data_output_dir: train-output
    data_path: ./taxonomy_data
    deepspeed_options:
      cpu_offload_optimizer: false
      cpu_offload_optimizer_pin_memory: false
      cpu_offload_optimizer_ratio: 1.0
      save_samples: null
    effective_batch_size: 3840
    is_padding_free: false
    learning_rate: 2e-6
    lora:
      alpha: 32
      dropout: 0.1
      quantize_data_type: null
      rank: 4
      target_modules:
      - q_proj
      - k_proj
      - v_proj
      - o_proj
    max_batch_len: 10000
    max_seq_len: 4096
    mock_data: false
    mock_data_len: 0
    model_path: instructlab/merlinite-7b-lab
    num_epochs: 10
    random_seed: 42
    save_samples: 250000
    warmup_steps: 800

and rather than adding an evaluate_serve config options is: the settings in _serve would be used for all commands implicitly starting servers

the whole process I have now for flattening the default_map can be expanded to take options from _serve, add them to _evaluate but only in the default flag map. Users could then override these options using the flags.

Some caveats would be stuff like max-ctx-size. Honestly stuff like this should probably just live in the _evaluate class and map directly to flags. The flatter the config the easier profiles and flag defaulting will be in the future. I think we need to get in the mental habit that whenever a top level config portion is edited like _serve, the next time any cmd that starts a server is run, it pulls from that cfg.

Creating sep _serve classes per command doesn't fit with the eventual "profile" goal of one class per cmd. I am open to any suggestions here though!

@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 1, 2024
@danmcp
Use new vllm/llama-cpp backends for evaluate
cc8cf35 
Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@danmcp

danmcp commented Jul 2, 2024

Copy link
Copy Markdown
Contributor Author

@cdoern I've rewritten to use the serve params and override with eval requirements. LMKWYT.

@alimaredia
alimaredia self-requested a review July 2, 2024 01:15
alimaredia
alimaredia previously approved these changes Jul 2, 2024
View reviewed changes
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 2, 2024
@danmcp danmcp mentioned this pull request Jul 2, 2024
Merged
3 tasks
cdoern
cdoern previously approved these changes Jul 2, 2024
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the right structure I think in terms of config. I might do a follow up to make the option overriding work as it does in the other cmds.

The way I set it up for train and @alimaredia did for serve is that in configuration.py the default_map in the context is set to have the right key->value pairs that will correspond to flag names in the specific cmd.

This is a clean solution though too and gets it all working, so it gets my +1 !

@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 2, 2024
leseb
leseb reviewed Jul 2, 2024
View reviewed changes

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but I think it's worth a note in the CHANGELOG.md. Thanks!

@danmcp
danmcp dismissed stale reviews from cdoern and alimaredia via 1f4c177 July 2, 2024 12:55
@mergify mergify Bot added ci-failure PR has at least one CI failure CI/CD Affects CI/CD configuration labels Jul 2, 2024
@danmcp
Update changelog
eefdc02 
Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 2, 2024
@mergify mergify Bot added one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 2, 2024
@cdoern

cdoern commented Jul 2, 2024

Copy link
Copy Markdown
Contributor

macos has been flaky recently, rerunning

@mergify mergify Bot removed one-approval PR has one approval from a maintainer ci-failure PR has at least one CI failure labels Jul 2, 2024
@cdoern cdoern mentioned this pull request Jul 2, 2024
Merged
@mergify
mergify Bot merged commit 08a9476 into instructlab:main Jul 2, 2024
@ktam3 ktam3 added this to the 0.18.0 milestone Jul 15, 2024
@danmcp
danmcp deleted the evalandvllm branch August 7, 2024 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers
@tiran tiran tiran left review comments
@leseb leseb leseb approved these changes
@alimaredia alimaredia alimaredia approved these changes
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
@cdoern cdoern cdoern left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CI/CD Affects CI/CD configuration testing Relates to testing

Projects

None yet

Milestone

0.18.0a1

Development

Successfully merging this pull request may close these issues.

7 participants

@danmcp @nathan-weinberg @cdoern @tiran @leseb @alimaredia @ktam3