Fixing bug 857 (regression from 0.1.14 to 0.1.15)

[FoxBuchele]

Fixing regression issue in 0.1.15 where capturing text would cause the LM to slice it incorrectly due to stop and start tokens.

[FoxBuchele]

Converted to draft because I noticed some errors when writing tests (specifically, guidance functions within f-strings were not converted properly).

Looking into fixing it the 'right' way, and will re-open the pull request then, unless someone else gets to it first.

[riedgar-ms]

Could you add a test which fails without your fix?

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.25%. Comparing base (4ae2ea6) to head (cf2bf6f).
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
+ Coverage   56.43%   61.25%   +4.81%     
==========================================
  Files          63       63              
  Lines        4791     4798       +7     
==========================================
+ Hits         2704     2939     +235     
+ Misses       2087     1859     -228     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[FoxBuchele]

I can definitely try!

The fix is only relevant/used in models that use role blocks (with system(), with assistant(), etc) that also have closing tags whenever the role is completed, so I'll have to dig into the test suite to determine how to write a test properly that only runs on some types of LMs.

[riedgar-ms]

I can definitely try!

The fix is only relevant/used in models that use role blocks (with system(), with assistant(), etc) that also have closing tags whenever the role is completed, so I'll have to dig into the test suite to determine how to write a test properly that only runs on some types of LMs.

Look at the selected_model_name and selected_model fixtures for that. You can see one approach here:

guidance/tests/models/test_llama_cpp.py

Line 11 in c9e71fb

def llamacpp_model(selected_model, selected_model_name):

[FoxBuchele]

I can definitely try!
The fix is only relevant/used in models that use role blocks (with system(), with assistant(), etc) that also have closing tags whenever the role is completed, so I'll have to dig into the test suite to determine how to write a test properly that only runs on some types of LMs.

Look at the selected_model_name and selected_model fixtures for that. You can see one approach here:

guidance/tests/models/test_llama_cpp.py

Line 11 in c9e71fb

def llamacpp_model(selected_model, selected_model_name):

Thanks!! That was very helpful.

I added a single basic test that ensures if you capture text within a role block, the text captured is sliced from the model at the appropriate location.

[riedgar-ms]

Is this the only model for which the test works? I thought that some of the others supported the role tags? Perhaps move the fixture to conf.py and call it something like model_with_role_tags ?

[FoxBuchele]

Ah it appears so! It looks like there's both transformers and non transformers of phi3 mini instruct 4k, as well as a couple other instruct versions I recognized.

Good idea to move it to the conftest.py file though, I can definitely see situations where we could use more tests specifically for models that utilize roles.

[riedgar-ms]

@FoxBuchele there seems to be a test failure related to this?

[riedgar-ms]

Slightly concerned that this appears to be relying on ChatML tags, which not all models use