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

json: unified properties order across optional & required #8133

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ochafik
Copy link
Collaborator

@ochafik ochafik commented Jun 26, 2024

This makes properties to be generated in the order they're defined, no matter whether they're optional or required

(Follow up to #7840 (comment), cc/ @HanClinto )

{
    "type": "object",
    "properties": {
        "a": { "type": "integer" },
        "b": { "type": "integer" },
        "c": { "type": "integer" },
        "d": { "type": "integer" }
    },
    "required": ["b", "d"]
}

Before this PR, required properties were generated first (and order is then following definition within group of required & optional props):

{"b": 0, "d": 0, "a": 0, "b": 0}
{"b": 0, "d": 0}

After, all properties appear in a unified order:

{"a": 0, "b": 0, "c": 0, "d": 0}
{"b": 0, "d": 0}

This has the added benefit of reducing the max number of parallel alternatives, which should speed grammar sampling up.

TODO:

@mofosyne mofosyne added the Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level label Jun 26, 2024
@github-actions github-actions bot added testing Everything test related examples python python script changes server labels Jun 26, 2024
@slaren
Copy link
Member

slaren commented Jun 27, 2024

Unrelated to this PR, but I am mentioning this here since you are working on this. The test-json-schema-to-grammar is failing regularly in the AVX512 CI due to hitting a 15 minute timeout, eg. https://github.com/ggerganov/llama.cpp/actions/runs/9701435352/job/26775088066. This test runs under an emulator if the runner doesn't have AVX512 support, so I think it is expected that tests will take longer than normal, but it shouldn't take that long. Any ideas about how to fix it?

@ochafik
Copy link
Collaborator Author

ochafik commented Jun 28, 2024

Unrelated to this PR, but I am mentioning this here since you are working on this. The test-json-schema-to-grammar is failing regularly in the AVX512 CI due to hitting a 15 minute timeout, eg. https://github.com/ggerganov/llama.cpp/actions/runs/9701435352/job/26775088066. This test runs under an emulator if the runner doesn't have AVX512 support, so I think it is expected that tests will take longer than normal, but it shouldn't take that long. Any ideas about how to fix it?

@slaren oh that's weird, looks like it's timing out in the node.js portion of the test, although without timestamped logs it's not clear if the C++ or (more likely) the Python versions are (also) to blame.

If the flakiness is an issue I'd opt to modify the LLAMA_NODE_AVAILABLE & python logic in the test to just let any CI runner relying on an emulator skip both the python & js branches (+ file a bug for me to investigate further, I've certainly optimised the latest changes for ease of writing & cross-language portability rather than speed 😅)

@ochafik
Copy link
Collaborator Author

ochafik commented Jun 28, 2024

Currently seems this is a bit slower than master (although weirdly with a bit of variability despite the --seed), against my expectations from #7840 (comment)

# git add remote ochafik https://github.com/ochafik/llama.cpp
# git fetch ochafik

python examples/json_schema_to_grammar.py \
  https://json.schemastore.org/tsconfig.json \
  > grammars/tsconfig.json.gbnf

hyperfine --warmup 1 --runs 10 \
  -L branch master,ochafik/json-order \
    --setup 'git checkout {branch} && \
             make clean && \
             make -j LLAMA_CURL=1 llama-cli' \
    'BRANCH={branch} \
      ./llama-cli --grammar-file grammars/tsconfig.json.gbnf \
        -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
        -mu https://huggingface.co/bartowski/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct-Q8_0.gguf \
        --seed 13345'
Benchmark 1: BRANCH=master \
      ./llama-cli --grammar-file grammars/tsconfig.json.gbnf \
        -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
        -mu https://huggingface.co/bartowski/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct-Q8_0.gguf \
        --seed 13345
  Time (mean ± σ):      8.894 s ±  0.702 s    [User: 1.177 s, System: 0.366 s]
  Range (min … max):    8.059 s … 10.106 s    10 runs
 
Benchmark 2: BRANCH=ochafik/json-order \
      ./llama-cli --grammar-file grammars/tsconfig.json.gbnf \
        -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
        -mu https://huggingface.co/bartowski/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct-Q8_0.gguf \
        --seed 13345
  Time (mean ± σ):      9.467 s ±  0.585 s    [User: 1.127 s, System: 0.342 s]
  Range (min … max):    8.363 s …  9.987 s    10 runs
 
Summary
  'BRANCH=master \
      ./llama-cli --grammar-file grammars/tsconfig.json.gbnf \
        -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
        -mu https://huggingface.co/bartowski/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct-Q8_0.gguf \
        --seed 13345' ran
    1.06 ± 0.11 times faster than 'BRANCH=ochafik/json-order \
      ./llama-cli --grammar-file grammars/tsconfig.json.gbnf \
        -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
        -mu https://huggingface.co/bartowski/Meta-Llama-3-8B-Instruct-GGUF/resolve/main/Meta-Llama-3-8B-Instruct-Q8_0.gguf \
        --seed 13345'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples python python script changes Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level server testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants